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

Convert UseSingularNouns to configurable rule and add Windows to allowlist #1858

Merged
merged 6 commits into from
Jan 18, 2024

Conversation

MJVL
Copy link
Contributor

@MJVL MJVL commented Nov 6, 2022

PR Summary

This PR:

Relevant test has also been added.

PR Checklist

Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

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

I'd be happy to accept it as-is but I am thinking it might be better to expose this allowList as a configurable setting so it can be overridden similar to how we decided to do for this rule when allow-listing az:
https://github.com/PowerShell/PSScriptAnalyzer/pull/1846/files#diff-0c1b69454498d51fc5f5ba732f2589578f37357dd80b111e8330fff529036908

@MJVL
Copy link
Contributor Author

MJVL commented Nov 7, 2022

I'd be happy to accept it as-is but I am thinking it might be better to expose this allowList as a configurable setting so it can be overridden similar to how we decided to do for this rule when allow-listing az: https://github.com/PowerShell/PSScriptAnalyzer/pull/1846/files#diff-0c1b69454498d51fc5f5ba732f2589578f37357dd80b111e8330fff529036908

I agree, as this could cut down on any future issues asking for additional allowList nouns. I can look into refactoring this as a configurable rule.

For the default value, do you think it would still be proper to include {"Data", "Windows"}, or would it be better practice to leave this blank entirely?

@bergmeister
Copy link
Collaborator

Yes, in that case also happy to add Windows to default.

@MJVL MJVL changed the title Add Windows to noun allowlist Convert UseSingularNouns to configurable rule and add Windows to allowlist Nov 8, 2022
docs/Rules/UseSingularNouns.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

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

Thanks for the additional effort. Looks good to me 👍🏻

@bergmeister
Copy link
Collaborator

Closing and re-opening to re-trigger CI

@bergmeister bergmeister closed this Jan 2, 2024
@bergmeister bergmeister reopened this Jan 2, 2024
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.

None yet

3 participants