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

OSOE-84: New words to allow for Node.js Extensions #186

Merged
merged 30 commits into from
Jan 24, 2023

Conversation

0liver
Copy link
Contributor

@0liver 0liver commented Jan 16, 2023

Oliver Friedrich added 23 commits December 8, 2022 21:24
- that happened because I had reverted the accidental merge of OSOE-84-final into dev
# Conflicts:
#	.github/actions/spelling/action.yml
#	.github/actions/spelling/excludes.txt
#	.github/actions/spelling/expect.txt
# Conflicts:
#	.github/actions/spelling/allow.txt
#	.github/actions/spelling/excludes.txt
#	.github/actions/spelling/expect.txt
#	Docs/Workflows.md
# Conflicts:
#	.github/actions/spelling/allow.txt
#	.github/actions/spelling/excludes.txt
# Conflicts:
#	.github/actions/spelling/allow.txt
# Conflicts:
#	.github/actions/spelling/allow.txt
@@ -8,17 +8,21 @@ apiclienttenant
apikey
apps
appsettings
argv
Copy link
Member

Choose a reason for hiding this comment

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

Some of these look generic enough for the common configuration, but some very rare. For the latter, rather use #spell-check-ignore-line

Copy link
Contributor Author

@0liver 0liver Jan 18, 2023

Choose a reason for hiding this comment

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

Actually, most of those rare words are simply specific to NE. Are we planning to support project-level dictionaries anytime soon (@BenedekFarkas)? Because then I'd rather move them into one of those than commenting them everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good point, the spelling configuration in most repositories (but definitely in OSOCE) will be completely overhauled in Lombiq/Open-Source-Orchard-Core-Extensions#346.
So, you can apply the current practices to make it easier on rolling this out without having to worry about upcoming changes.

See my comment about using per-line ignores here: #134 (comment)
If you can expect that a specific word will appear in more places in the future, then putting it in a dictionary is OK. Based on applying this concept to OrchardCMS/OrchardCore.Commerce#250 and some other repos, I'd say that maximum 1 or 2 occurrences of a rare word (if you don't expect more) is OK to ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record - all errors by the spell checker when no new entries are added:

Autoprefix is not a recognized word. (unrecognized-spelling)
EBUSY is not a recognized word. (unrecognized-spelling)
EOL is not a recognized word. (unrecognized-spelling)
EOL is not a recognized word. (unrecognized-spelling)
EOL is not a recognized word. (unrecognized-spelling)
Lintrc is not a recognized word. (unrecognized-spelling)
Lintrc is not a recognized word. (unrecognized-spelling)
Promisified is not a recognized word. (unrecognized-spelling)
Resx is not a recognized word. (unrecognized-spelling)
argv is not a recognized word. (unrecognized-spelling)
argv is not a recognized word. (unrecognized-spelling)
argv is not a recognized word. (unrecognized-spelling)
argv is not a recognized word. (unrecognized-spelling)
argv is not a recognized word. (unrecognized-spelling)
binstubs is not a recognized word. (unrecognized-spelling)
chdir is not a recognized word. (unrecognized-spelling)
chdir is not a recognized word. (unrecognized-spelling)
chdir is not a recognized word. (unrecognized-spelling)
chdir is not a recognized word. (unrecognized-spelling)
constitue is not a recognized word. (unrecognized-spelling)
copyfiles is not a recognized word. (unrecognized-spelling)
copyfiles is not a recognized word. (unrecognized-spelling)
copyfiles is not a recognized word. (unrecognized-spelling)
copyfiles is not a recognized word. (unrecognized-spelling)
copyfiles is not a recognized word. (unrecognized-spelling)
errored is not a recognized word. (unrecognized-spelling)
lintrc is not a recognized word. (unrecognized-spelling)
loglevel is not a recognized word. (unrecognized-spelling)
npmrc is not a recognized word. (check-file-path)
npmrc is not a recognized word. (check-file-path)
npmrc is not a recognized word. (unrecognized-spelling)
promisify is not a recognized word. (unrecognized-spelling)
promisify is not a recognized word. (unrecognized-spelling)
symlink is not a recognized word. (unrecognized-spelling)
symlink is not a recognized word. (unrecognized-spelling)
symlinked is not a recognized word. (unrecognized-spelling)
symlinked is not a recognized word. (unrecognized-spelling)
symlinks is not a recognized word. (unrecognized-spelling)
textlint is not a recognized word. (unrecognized-spelling)
textlint is not a recognized word. (unrecognized-spelling)
textlint is not a recognized word. (unrecognized-spelling)
thenable is not a recognized word. (unrecognized-spelling)

Copy link
Member

Choose a reason for hiding this comment

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

That's not too bad! Just add them here to allow.txt for now (or ignore in-code whatever you see fit for that) and I'll take care of the rest during the refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did what Zoltán suggested - ignored the rare ones, re-added the common ones.

@sarahelsaig sarahelsaig merged commit 6eab63a into dev Jan 24, 2023
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.

4 participants