feat(eslint-plugin): [member-accessibility] add more options #322
Conversation
Adds configuration object to match options defined in #214 Adds implementation for the no public option at the root of the config object Adds implement for the overrides object
Adds option for parameterProperty validation Updates documentation to cover the new options
There was a problem hiding this comment.
I'm sorry to be a pain and flip-flop here considering that I suggested this options schema to you.
The more I think about it, the more I think it's better if we flatten the options.
Would be good to get thoughts from @typescript-eslint/core-team.
Current schema:
/*
false = don't check
true = check and use base config
object = override base config
*/
type Override = boolean | {
noPublic?: boolean;
};
type Options = [
{
noPublic?: boolean;
overrides?: {
accessor?: Override;
constructor?: Override;
method?: Override;
parameterProperty?: Override;
}
}
]Maybe it's better if it's:
type AccessibilityLevel =
| 'explicit' // require an accessor (including public)
| 'no-public' // don't require public
| 'off'; // don't check
type Options = [
{
accessibility?: AccessibilityLevel;
overrides?: {
accessor?: AccessibilityLevel;
constructor?: AccessibilityLevel;
method?: AccessibilityLevel;
parameterProperty?: AccessibilityLevel;
}
}
]I know that this isn't "future-proof" (i.e. it's not easy to add extra options), but it's probably better to optimise for ease of configuration for the user instead of optimising for potential future options?
packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
Outdated
Show resolved
Hide resolved
Simplified options and cleaned up comments
|
Thanks for the feedback. The new options are much nicer. |
|
I don't think it's worth doing a breaking change for this. If we hate the name we can always rename it later when we look at renaming a bunch of other rules. |
packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
Outdated
Show resolved
Hide resolved
Cleaned up override reading Simplified reportIssue signature
|
@bradzacher do you think we can move on with this PR so it would be released in the next version? |
|
@mistic - For feature PRs, we generally look for 2 reviews from members before merging. Myself and the other members are all volunteers, so it's not unusual that any number of us can get caught up in our professional/personal lives and not be able to work on FOSS projects for a number of weeks. |
|
@bradzacher thanks for letting me know, that makes totally sense! |
Codecov Report
@@ Coverage Diff @@
## master #322 +/- ##
==========================================
- Coverage 97.08% 96.96% -0.12%
==========================================
Files 69 69
Lines 2537 2572 +35
Branches 395 408 +13
==========================================
+ Hits 2463 2494 +31
- Misses 45 46 +1
- Partials 29 32 +3
|
Hi,
This PR is a first draft of the options to allow parity with ts-lint and the member-accessibility options as discussed in #214
Overrides for parameter-properties are now included.
Docs are updated.
This feels like the rule needs to be re-named, perhaps to
member-accessibilitywhich would represent a breaking change.