Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add buyer/seller IDs to CSV fields #302

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

b-r-u
Copy link

@b-r-u b-r-u commented Nov 25, 2021

Previously, the CSV exports used to only contain the names of the
trading parties. Including the unique IDs makes it easier to connect
CSV rows to the corresponding entities.

Previously, the CSV exports used to only contain the names of the
trading parties. Including the unique IDs makes it easier to connect
CSV rows to the corresponding entities.
@CLAassistant
Copy link

CLAassistant commented Nov 25, 2021

CLA assistant check
All committers have signed the CLA.

@hannesdiedrich
Copy link
Member

Thanks a lot for your contribution and re-adding the buyer and seller ids to the exported CSV files. We will leave our reviews soon.

@hannesdiedrich hannesdiedrich requested a review from a team December 13, 2021 09:39
@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.58%. Comparing base (09a0204) to head (d7f4117).
Report is 1037 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #302   +/-   ##
=======================================
  Coverage   52.58%   52.58%           
=======================================
  Files          50       50           
  Lines        3041     3041           
  Branches      586      586           
=======================================
  Hits         1599     1599           
  Misses       1371     1371           
  Partials       71       71           

Copy link
Member

@hannesdiedrich hannesdiedrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@b-r-u
Copy link
Author

b-r-u commented Dec 15, 2021

Thanks for taking the time to review this external PR! Should I update the branch with the newest changes from master?

@Hassan754
Copy link
Contributor

Thanks for taking the time to review this external PR! Should I update the branch with the newest changes from master?

Yes, before pushing a PR to master it should be up to date, let us know when you're done so we can perform another round of review

@b-r-u
Copy link
Author

b-r-u commented Dec 15, 2021

@Hassan754 Alright, there were no merge conflicts with master. The files I edited remained untouched since my commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants