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

* fixes #92 converted Table to a namedtuple #94

Merged
merged 2 commits into from
Feb 18, 2019

Conversation

Aylr
Copy link
Contributor

@Aylr Aylr commented Feb 18, 2019

No description provided.

@CLAassistant
Copy link

CLAassistant commented Feb 18, 2019

CLA assistant check
All committers have signed the CLA.

@ManuelAlvarezC ManuelAlvarezC self-assigned this Feb 18, 2019
@ManuelAlvarezC ManuelAlvarezC added the internal The issue doesn't change the API or functionality label Feb 18, 2019
@@ -147,7 +143,8 @@ def _anonymize_data(self):

pii_fields = self.ht._get_pii_fields(ht_meta)
if pii_fields:
table.data = ht_table
# Table is a namedtuple, which is immutable, so instantiate a new one with transformed data
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is too long and makes the TravisCI build crash, but a part from this, all your changes seems good to merge, could you please shorten it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also a tip: If you want to check for linting issues on your local environment before open the PR, you can do it with make lint, or run the same build that TravisCI does with make test-all. In case you want further reference, please find our Contributing Guide here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up - I found the contributing guide after I had used travis to debug. 😄

@ManuelAlvarezC
Copy link
Contributor

Hi @Aylr and thanks for your contribution!

I'll proceed to merge the PR after this message, but before that I wanted to ask you for a little favor:

Next time you collaborate with this project, would you mind commenting in the original issue before starting implementing it?

This way we can assign it to you and avoid the unpleasant chance that when your changes are ready, somebody else has taken it.

Thanks in advance for your comprehension :)

@ManuelAlvarezC ManuelAlvarezC merged commit a2e3f70 into sdv-dev:master Feb 18, 2019
@Aylr
Copy link
Contributor Author

Aylr commented Feb 18, 2019

Will do!

@Aylr Aylr deleted the Aylr/convert-table-to-named-tuple branch February 18, 2019 20:15
@ManuelAlvarezC
Copy link
Contributor

Hi @Aylr

Sorry to bother you once again, but would you mind sharing your name and email to be included in the Contributors of the project?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal The issue doesn't change the API or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants