-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
STYLE: Use f-string in io/parsers #30466
Conversation
…the string at line 1501. That still requires work. #29886
Hello @naomi172839! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-12-25 22:34:39 UTC |
Thanks for the work on this @naomi172839 If you check your proposed changes in https://github.com/pandas-dev/pandas/pull/30466/files you'll see that there are many unrelated changes (changes in the indentation). To keep things simple, we don't merge PRs with side changes. And in this case, they are also incorrect based on Python standards. I guess the changes were done automatically by your editor, or a different version of black than you should have. In any case, please have a look and make sure that you're only proposing the intended changes. Let me know if you need help. |
I believe that pycharm automatically formats the code. I will correct the issue and submit a new pull request. |
I was guessing is the editor. I don't know anything about PyCharm, but you may want to disable whatever is modifying the code automatically. You don't need to create a new PR. Make the changes locally in your branch, commit them, and push them. For the next time, you should create a git branch before starting working on something you want to propose. Working on |
…the string at line 1501. That still requires work. #29886 -- Updated with "Black" Styling
@datapythonista Thank you for the advice! I ran "black" on the file and recommitted it. Hopefully that is all that I needed to do. Also thank you for your time! |
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.
Thanks for the work on this @naomi172839, really nice clean up.
Added some comments, but looks good
also removed some unnecessary f-strings
pandas/io/parsers.py
Outdated
@@ -710,6 +708,7 @@ def read_fwf( | |||
**kwds, | |||
): | |||
|
|||
|
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.
You're adding a blank line now.
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.
removed the line. Hopefully everything is in line with PEP8 now. Sorry that this created so much work for you! And thank you for walking me through it.
also removed some unnecessary f-strings
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.
Looks good, just couple of small things, but almost ready to be merged.
Don't worry about the work on reviewing. Iterating few times is normal and expected when people is starting. Keep contributing as much as you want, we're happy to review.
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 for the work on this @naomi172839, nice PR.
Thanks again @datapythonista! Now that everything passes is there anything else that I am supposed to do? |
No, now someone else will have a look, and either have comments, or will merge this. We like to have two people from the team reviewing all the changes, that's why I didn't merge this myself (I approved, so next maintainer can merge if they are happy). |
thanks @naomi172839 |
xref #29547
Fixed many of the strings in pandas/io/parsers.py except for the string at line 1501 as that will require more time to make it function properly. Please note that this is my first contribution ever so please double check my work!