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

feat(eslint-config): Update constructor access modifier eslint rule to require explicit modifiers #13134

Merged

Conversation

Josmithr
Copy link
Contributor

@Josmithr Josmithr commented Nov 30, 2022

Updates rule to match discussion outcome in this discussion.

Rule documentation: https://typescript-eslint.io/rules/explicit-member-accessibility/

Also fixes violations in packages using the strict config

Breaking Change

Note that this is a breaking change with respect to the above rule. We are changing error-level enforcement of the policy to the opposite rule, so code changes will be required. This PR pre-fixes the violations existing at this point in time, but any changes between now and when the new package is published and adopted will need to be fixed.

@Josmithr Josmithr requested review from a team as code owners November 30, 2022 19:36
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: driver Driver related issues area: examples Changes that focus on our examples base: main PRs targeted against main branch and removed area: examples Changes that focus on our examples area: driver Driver related issues area: dds Issues related to distributed data structures labels Nov 30, 2022
@Josmithr Josmithr changed the title Update constructor access modifier rule eslint to require explicit modifiers Update constructor access modifier eslint rule to require explicit modifiers Nov 30, 2022
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: driver Driver related issues area: examples Changes that focus on our examples labels Nov 30, 2022
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Nov 30, 2022

Could not find a usable baseline build with search starting at CI b54097b

Generated by 🚫 dangerJS against 8bc4142

@Josmithr Josmithr changed the title Update constructor access modifier eslint rule to require explicit modifiers feat: Update constructor access modifier eslint rule to require explicit modifiers Nov 30, 2022
Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

Looks good to me other than the single comment below.

packages/drivers/local-driver/src/localSessionStorageDb.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ChumpChief ChumpChief left a comment

Choose a reason for hiding this comment

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

aw yeah

@github-actions github-actions bot removed the area: driver Driver related issues label Dec 1, 2022
@Josmithr
Copy link
Contributor Author

Josmithr commented Dec 1, 2022

Waiting on ongoing discussion about major version changes in this and other individually published packages.

Update: PR pre-emptively major-bumping the eslint config for this and other breaking changes: #13218

Update 2: Above PR merged. This is ready to go.

@Josmithr Josmithr changed the title feat: Update constructor access modifier eslint rule to require explicit modifiers feat(eslint-config): Update constructor access modifier eslint rule to require explicit modifiers Dec 1, 2022
@Josmithr Josmithr requested a review from ChumpChief December 2, 2022 23:15
@Josmithr Josmithr merged commit faa9111 into microsoft:main Dec 5, 2022
@Josmithr Josmithr deleted the update-constructor-access-rule-eslint branch December 5, 2022 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds Issues related to distributed data structures area: examples Changes that focus on our examples base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants