-
-
Notifications
You must be signed in to change notification settings - Fork 305
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
327 add documentation for dataframeschema transformations #333
327 add documentation for dataframeschema transformations #333
Conversation
thanks @ktroutman! looks like there are a few lines that need to be re-formatted to stay <= 79 chars: I think we should also edit this part of the sphinx docs to point users to the API reference: I'll leave specifics up to you, but something like:
Then at the end of the section
|
hey FYI @ktroutman I'm about to merge a big changeset from The issues discussed in #333 (comment) should also be addressed in this PR. Let me know if you need any help! |
Hi @cosmicBboy , ok I'm going to submit the latest changes to this PR tonight, which include a bunch of docs updates, a possible bug fix that i found in rename_columns, and the update columns function. I just have to iron out the tests for update_columns and then should be good to go. If that's too much to pack in for this merge, then I can split the update_columns and the bug fix into separate PRs. |
… update columns add, some suggested helpful hints for helping with documentation in the CONTRIBUTING.md
@@ -19,6 +19,21 @@ create a development environment that is separate from your existing Python | |||
environment so that you can make and test changes without compromising your | |||
own work environment. | |||
|
|||
### Contributing documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought
) | ||
|
||
new_columns: Dict[str, Dict[str, Any]] = {} | ||
for col in new_schema.columns: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to sleeker implementations
rename_dict[col_name] if col_name in rename_dict else col_name | ||
): col_attrs | ||
for col_name, col_attrs in self.columns.items() | ||
(rename_dict[col_name] if col_name in rename_dict else col_name): ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous implementation left the name unchanged. So if you changed a name from col1 to col2, the name parameter remained col1. I understand why changing names is undesirable, but this is surely not intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, nice! thanks for catching this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks @ktroutman! 🚀
Codecov Report
@@ Coverage Diff @@
## master #333 +/- ##
==========================================
- Coverage 98.79% 98.70% -0.10%
==========================================
Files 18 18
Lines 1747 1780 +33
==========================================
+ Hits 1726 1757 +31
- Misses 21 23 +2
Continue to review full report at Codecov.
|
Here is some basic documentation for the verb functions.
I also added some error handling on a few of the other functions, since for some, if you passed a non-existing column it would just return the original dataframeschema, without throwing an error.