Skip to content

README: add --ignore-words-list - #2040

Open
kolyshkin wants to merge 1 commit into
codespell-project:mainfrom
kolyshkin:patch-1
Open

kolyshkin wants to merge 1 commit into
codespell-project:mainfrom
kolyshkin:patch-1

Conversation

@kolyshkin

@kolyshkin kolyshkin commented Aug 11, 2021 •

Copy link
Copy Markdown

It took me a while to figure out how to specify a list of ignored words
in a configuration file, as --ignore-words-list was missing from README
(I eventually found it elsewhere).

  • added --ignore-words-list long option next to -L
  • added ignore-words-list to the .codespellrc example
  • improved and simplified options description
  • simplified literal blocks formatting
  • in .codespellrc example, made options order match that of cli example

@peternewman peternewman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this.

Regarding the options with arguments, looking at the grep man page for example, it looks like this:
-e PATTERN, --regexp=PATTERN

It would seem sense to me to follow the same style so would you mind updating them to match

Comment thread README.rst Outdated
Comment thread README.rst Outdated
@kolyshkin

Copy link
Copy Markdown
Author

@peternewman PTAL

@peternewman
peternewman requested a review from luzpaz September 21, 2021 14:02

@peternewman peternewman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this new more succinct wording, but I've asked @luzpaz for a review too as they had tweaked this in the past.

Comment thread README.rst
Comment thread README.rst Outdated
@kolyshkin

Copy link
Copy Markdown
Author

Sorry, I have used markdown formatting while describing the FILE and LIST arguments earlier, and this is an rst file.

Fixed now.

@kolyshkin
kolyshkin force-pushed the patch-1 branch 3 times, most recently from 187c37c to eecac55 Compare September 23, 2021 00:07

@peternewman peternewman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor comment

Comment thread README.rst Outdated
@kolyshkin

Copy link
Copy Markdown
Author

Is there anything else I need to address here? @peternewman @luzpaz

@peternewman peternewman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great thanks @kolyshkin . Just some minor tidying suggestions.

Also none of the actions have run for some reason so I'm going to try and close and open the PR to see if it kicks them off...

Comment thread README.rst Outdated
Comment thread README.rst Outdated
Comment thread README.rst Outdated
@peternewman peternewman reopened this Oct 25, 2021
@kolyshkin
kolyshkin force-pushed the patch-1 branch 9 times, most recently from 184400d to ea9bfe3 Compare January 3, 2022 21:38
@kolyshkin

Copy link
Copy Markdown
Author

Rebased; addressed review comments; PTAL @peternewman

@peternewman peternewman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one minor tweak

Comment thread README.rst Outdated
@peternewman
peternewman requested review from luzpaz and removed request for luzpaz January 4, 2022 01:36
@peternewman

Copy link
Copy Markdown
Collaborator

@luzpaz do you want to take a look too as you've tweaked this in the past?

@kolyshkin

Copy link
Copy Markdown
Author

I am pretty sure there are many more improvements that can be done to the README in this repo. Originally my intention was to merely add --ignore-words-list (because I spent quite some time looking for it, and wanted others to find it faster/easier). It is getting out of hand now as I am adding more and more fixes on top, and already had to rebase this once. The fact that I am not very familiar with ReST syntax doesn't help either 😓.

Can this be merged as is (unless there are some incorrect things that this PR adds), and implement more improvements separately on top of this?

PS if you want to see how this is rendered (by Github, that is), see https://github.com/kolyshkin/codespell/blob/patch-1/README.rst.

@peternewman

Copy link
Copy Markdown
Collaborator

I am pretty sure there are many more improvements that can be done to the README in this repo. Originally my intention was to merely add --ignore-words-list (because I spent quite some time looking for it, and wanted others to find it faster/easier). It is getting out of hand now as I am adding more and more fixes on top, and already had to rebase this once.

Yeah sorry, just spotting related bits in the course or reviewing your PR.

Can this be merged as is (unless there are some incorrect things that this PR adds), and implement more improvements separately on top of this?

Yes certainly. I'd like to give @luzpaz a little while to take a look, but happy to merge if they've not reviewed soon.

PS if you want to see how this is rendered (by Github, that is), see https://github.com/kolyshkin/codespell/blob/patch-1/README.rst.

Yeah, I'd had a look already and it looks fine to me thanks!

@luzpaz

luzpaz commented Jan 5, 2022

Copy link
Copy Markdown
Collaborator

Thanks for the refinements. LGTM

@kolyshkin

Copy link
Copy Markdown
Author

Just found this old PR which I spent so much time on and addressed every concern, yet it was not merged (probably merely forgotten).

This was all manual editing in pre-AI times. Let me try to revive this.

It took me a while to figure out how to specify a list of ignored words
in a configuration file, as --ignore-words-list was missing from README
(I eventually found it elsewhere).

* add --ignore-words-list long option next to -L;
* add ignore-words-list to the config file examples;
* add argument to -S, --skip;
* improve and simplify options description.

[v2: make config file options order match that in cli example]
[v3: more description changes as suggested by reviewers]
[v4: rebase; drop changes made obsolete by upstream README rework]

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin

Copy link
Copy Markdown
Author

Rebased onto current main. Since the README was reworked in the meantime, I redid the change on top of it.

Kept:

  • -L LIST, --ignore-words-list=LIST instead of -L word1,word2,word3,word4; long options are also mentioned in the "Ignoring words" section;
  • -S LIST, --skip=LIST instead of -S, --skip=;
  • ignore-words-list added to the INI and TOML config examples, and to the equivalent command line;
  • shorter descriptions for -I, -L, and -S.

Dropped, as made obsolete by upstream changes:

  • -w description rewording (it has since been updated, and --exit-non-zero-on-fix was added);
  • case-sensitivity notes (they now live in the "Ignoring words" section);
  • config options reordering (INI and CLI examples already use the same order);
  • literal blocks formatting (now code-block is used throughout).

@peternewman @luzpaz PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants