-
Notifications
You must be signed in to change notification settings - Fork 384
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
[Merged by Bors] - chore: Improved nth_rewrite and nth_rw docstrings #13877
Conversation
PR summary 7952c9c003Import changesNo significant changes to the import graph Declarations diffNo declarations were harmed in the making of this PR! 🐙 You can run this locally as follows## summary with just the declaration names:
./scripts/no_lost_declarations.sh short <optional_commit>
## more verbose report:
./scripts/no_lost_declarations.sh <optional_commit> |
Co-authored-by: Eric Wieser <[email protected]>
Co-authored-by: Eric Wieser <[email protected]>
Co-authored-by: Eric Wieser <[email protected]>
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 whole change is much welcome. As a new user I can understand the text, and Grammarl y only complains about "afterward(s)".
That's some Americanism by grammarly. "Afterwards" is definitely correct in English. See : https://dictionary.cambridge.org/thesaurus/articles/afterwards |
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!
Co-authored-by: Ruben Van de Velde <[email protected]>
LGTM. Another maintainer can review the above unresolved conversations to see if they feel anything should be done. Make sure to wait for CI (probably doesn't matter since it's only docstrings, but still). maintainer merge |
🚀 Pull request has been placed on the maintainer queue by j-loreaux. |
what is a |
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.
@semorrison I'm okay either way, applying your suggestion to copy the docs or not. Do you want to make the final decision?
…ural Co-authored-by: Anne Baanen <[email protected]>
@Shreyas4991 a |
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.
I resolved the open suggestion, since I don't consider them necessary and other reviewers didn't either.
I read through the text again and it's really clear. Left some comma suggestions, but I think it should be merged with or without them
Co-authored-by: Jon Eugster <[email protected]>
Co-authored-by: Jon Eugster <[email protected]>
Co-authored-by: Jon Eugster <[email protected]>
Line length adjustments
remove accidental spaces at end of line. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
maintainer merge
🚀 Pull request has been placed on the maintainer queue by Ruben-VandeVelde. |
bors merge |
Pull request successfully merged into master. Build succeeded: |
Zulip thread of discussion