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

Support list of wordlists as a string #170

Closed
wants to merge 9 commits into from

Conversation

grhm
Copy link
Contributor

@grhm grhm commented May 26, 2022

Allow the spelling_word_list_filename to be passed as a string, but still be a list filenames and treated as such

Fix for issue #169

Allow a wordlist filename string to contain a list of wordlist filenames
@dhellmann
Copy link
Member

So the syntax for the list is semi-Python, using the names without quotes separated by commas and wrapped in square brackets?

-D spelling_word_list_filename=[filename.txt,another-filename.txt]

How common do you think commas are in filenames? Could we use commas to separate the values and forego the brackets?

-D spelling_word_list_filename=filename.txt,another-filename.txt

That sort of syntax is pretty common in other tools I'm familiar with.

Other options would be to pass a directory name or glob pattern. Would either of those work for your use case?

@grhm
Copy link
Contributor Author

grhm commented May 30, 2022

I was trying to do as little as possible to not break things I haven't tested, so having square bracket around a list seemed kinda Pythony.

I'm not sure how common commas in file names are - and happy that foregoing the bracket and splitting on commas would work. If someone does have a comma in their filename, is fairly non-standard, and can be worked around by renaming the file.

A directory name or glob pattern wouldn't work in my case, as the "common" and "per-project" word lists end up in different locations.

@dhellmann
Copy link
Member

I was trying to do as little as possible to not break things I haven't tested, so having square bracket around a list seemed kinda Pythony.

I'm not sure how common commas in file names are - and happy that foregoing the bracket and splitting on commas would work. If someone does have a comma in their filename, is fairly non-standard, and can be worked around by renaming the file.

Let's go ahead with that, then. Updating the docs in docs/source/customize.rst and adding an entry to docs/source/history.rst would be good additions to the parsing change. A unit test for get_wordlist_filename() would be good, since the logic is growing more complex.

Do you want to make the updates to this PR?

A directory name or glob pattern wouldn't work in my case, as the "common" and "per-project" word lists end up in different locations.

That makes sense. Someone can always rely on the shell to do the globbing for them, anyway.

@dhellmann dhellmann self-assigned this May 30, 2022
@grhm
Copy link
Contributor Author

grhm commented May 30, 2022

Let's go ahead with that, then. Updating the docs in docs/source/customize.rst and adding an entry to docs/source/history.rst would be good additions to the parsing change. A unit test for get_wordlist_filename() would be good, since the logic is growing more complex.

Do you want to make the updates to this PR?

I've made changes to the rst files as requested - but have not added unit tests.
I've not sure how your unit tests work - happy for you to add them as you see fit.

@dhellmann
Copy link
Member

Let's go ahead with that, then. Updating the docs in docs/source/customize.rst and adding an entry to docs/source/history.rst would be good additions to the parsing change. A unit test for get_wordlist_filename() would be good, since the logic is growing more complex.
Do you want to make the updates to this PR?

I've made changes to the rst files as requested - but have not added unit tests. I've not sure how your unit tests work - happy for you to add them as you see fit.

No problem, I'll add some tests.

Copy link
Member

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

Thanks!

@dhellmann
Copy link
Member

I rebased this in #178

mergify bot added a commit that referenced this pull request May 30, 2022
@dhellmann dhellmann closed this May 30, 2022
@grhm grhm deleted the allow-string-list branch June 5, 2023 16:02
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.

None yet

2 participants