-
Notifications
You must be signed in to change notification settings - Fork 465
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
Add note about --exclude-file in README and update the help text also #2052
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.
I think I prefer your wording, but it would probably make sense if it matched the help file:
https://github.com/codespell-project/codespell/blob/d598d5b/codespell_lib/_codespell.py#L347-L349
Or can you at least add your second sentence to the command help please.
@peternewman I've updated both sets of text to try and bring them closer together; how does that look? |
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.
Perhaps also take a look at @kolyshkin work in https://github.com/codespell-project/codespell/pull/2040/files , I suspect their formatting of the METAVAR FILE should make things more readable (in the README file).
formatted the |
I suspect fewer words makes it easier to read, so that's probably a good idea. |
Can I suggest something like
|
thanks! I've updated it; how does that look? |
@bwitt can you please squash your commits? |
squashed |
@peternewman how does this look? |
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 one minor tweak from me.
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.
I'll just give other people a chance to take a look before merging.
@kolyshkin @DimitriPapadopoulos do you want to take another look? |
It looks good to me. It is now much harder to misunderstand as line numbers. |
@peternewman rebased and squashed to one commit; please let me know if there's anything else I need to do. |
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
Add a quick note about using
--exclude-file
to exclude whole lines, and update the help text too