-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Possible typo in CODEMODS.md #108
Conversation
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.
That indeed looks silly of me. It's possible I meant to do sorted(sorted(iterable), reverse=True)
=> sorted(iterable, reverse=True)
for the second line.
Demonstrating both sorted(sorted(iterable, reverse=True))
=> sorted(iterable)
and sorted(sorted(iterable, reverse=False))
=> sorted(iterable)
doesn't seem worthwhile, so I think the line (in both the input & output) should either be removed or maybe changed to the former. I'll leave it up to you which option you prefer :)
I've just updated my branch - I agree I also realised the same lines are present in (Annoyingly, GitHub is behaving strangely for me currently and shows no updates, only a 'processing updates' spinner at the top of the PR: I assume this is a GitHub problem that will resolve - I can see the commit is updated if I click through to my branch and view the diff with upstream.) |
oh, I probably just copy-pasted the lines from the tests to the doc. I'm seeing the same spinner and no update. You could try adding an empty commit ( (I'd generally recommend having multiple commits instead of amending and force-pushing regardless, it's less dangerous if others might be working on the same branch, and allows reviewers of a PR to be able to only review new changes in the github interface. Though don't know if that would've avoided github being weird here.) |
Good suggestion, all looking good now! Yes, I think I'm in bad habits there with force-pushing 😬 |
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 @EFord36! The little things like this really add up ❤️
Hi,
While reading through
CODEMODS.md
, I noticed two lines in thereplace_unnecessary_nested_calls
section that are identical, in a way that seems unintentional.I think maybe there was a typo of
True
forFalse
here (based on the following two lines), but could easily be wrong - I thought about opening this as an issue instead of a PR, but I believe there's some reasonable chance my change is appropriate, and it's low-cost if it's wrong (I'm happy if you say 'ah yes, there is a problem, you've fixed it wrong, I've fixed it myself, closing this PR).