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

Possible typo in CODEMODS.md #108

Merged
merged 2 commits into from
Apr 10, 2024
Merged

Possible typo in CODEMODS.md #108

merged 2 commits into from
Apr 10, 2024

Conversation

EFord36
Copy link
Contributor

@EFord36 EFord36 commented Apr 9, 2024

Hi,

While reading through CODEMODS.md, I noticed two lines in the replace_unnecessary_nested_calls section that are identical, in a way that seems unintentional.

I think maybe there was a typo of True for False 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).

Copy link
Contributor

@jakkdl jakkdl left a 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 :)

@EFord36
Copy link
Contributor Author

EFord36 commented Apr 10, 2024

I've just updated my branch - I agree sorted(sorted(iterable), reverse=True) makes more sense so changed to that.

I also realised the same lines are present in tests/recorded/comprehensions/C414.txt so I changed it there too.

(Annoyingly, GitHub is behaving strangely for me currently and shows no updates, only a 'processing updates' spinner at the top of the PR:

Screenshot 2024-04-10 at 10 21 54

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.)

@jakkdl
Copy link
Contributor

jakkdl commented Apr 10, 2024

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 (git commit --allow-empty) and we'll squash the PR.

(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.)

@EFord36
Copy link
Contributor Author

EFord36 commented Apr 10, 2024

Good suggestion, all looking good now!

Yes, I think I'm in bad habits there with force-pushing 😬

Copy link
Owner

@Zac-HD Zac-HD left a 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 ❤️

@Zac-HD Zac-HD merged commit f56ab96 into Zac-HD:master Apr 10, 2024
8 checks passed
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.

None yet

3 participants