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 addfields basic transformation #417

Merged
merged 4 commits into from
Aug 6, 2019

Conversation

mjumbewu
Copy link
Contributor

I've found it handy a few times to be able to add a bunch of fields in one go. Adding multiple fields at once saves the transformations from having to copy each row to a new view more times than it needs to.

This patch uses an implementation that I put together, closely mirroring addfield. Thoughts?

@alimanfoo
Copy link
Collaborator

Hi @mjumbewu, this is really nice. I'd be happy to add this new function.

My only thought is that it would be more straightforward to just use tuples, rather than have the field function and the FieldDefinition class. Each of the tuples could be of length 2 (field name, value) or length 3 (field name, value, index). So the logic is then just to check the length of the tuple.

@mjumbewu
Copy link
Contributor Author

@alimanfoo No problem. Also noting that the syntax def addfields(table, *fields, missing=None) breaks python2. I'm thinking of just changing it to def addfields(table, fields, missing=None), where fields is a collection of tuples, rather than moving the expansion to the end or just using *args, **kwargs.

@alimanfoo
Copy link
Collaborator

alimanfoo commented Mar 19, 2017 via email

- Add multiple fields to a table at once
- Adding multiple fields at once saves the transformations from having
  to copy each row to a new view more times than it needs to
@coveralls
Copy link

coveralls commented Aug 6, 2019

Coverage Status

Coverage decreased (-1.2%) to 89.921% when pulling d62bf6c on mjumbewu:addfields into 4195e3f on petl-developers:master.

@alimanfoo
Copy link
Collaborator

Going ahead with merge, happy to revisit if any comments.

@alimanfoo alimanfoo merged commit c39335d into petl-developers:master Aug 6, 2019
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.

3 participants