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.

Sign up for GitHub

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

Jump to bottom

feat(eslint-plugin): add preferProtectedState rule #4488

Merged
merged 8 commits into from Aug 15, 2024
Merged

feat(eslint-plugin): add preferProtectedState rule #4488

merged 8 commits into from Aug 15, 2024

Conversation

ghost
Copy link

@ghost ghost commented Aug 10, 2024 •
edited by ghost
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

#4474

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

See #4474

Closes #4474

What is the new behavior?

See #4474

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

obraz

obraz

@netlify Netlify
Copy link

netlify bot commented Aug 10, 2024 •
edited
Loading

✅ Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 2e45c0e
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/66bdc45867050d0008651ad6

rainerhahnekamp
rainerhahnekamp reviewed Aug 11, 2024
View reviewed changes
Copy link
Contributor

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

Well done @NgDaddy. I've just left a few potential textual improvements.

What we should discuss, if it should be fixable and if it should be of type 'problem'.

...ects/ngrx.io/content/guide/eslint-plugin/rules/signal-store-should-prefer-protected-state.md Outdated Show resolved Hide resolved
...ects/ngrx.io/content/guide/eslint-plugin/rules/signal-store-should-prefer-protected-state.md Outdated Show resolved Hide resolved
modules/eslint-plugin/src/rules/signals/signal-store-should-prefer-protected-state.ts Outdated Show resolved Hide resolved
modules/eslint-plugin/src/rules/signals/signal-store-should-prefer-protected-state.ts Outdated Show resolved Hide resolved
modules/eslint-plugin/src/rules/signals/signal-store-should-prefer-protected-state.ts Outdated Show resolved Hide resolved
modules/eslint-plugin/src/rules/signals/signal-store-should-prefer-protected-state.ts Outdated Show resolved Hide resolved
modules/eslint-plugin/spec/rules/signals/signal-store-should-prefer-protected-state.spec.ts Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Aug 11, 2024

Many thanks @rainerhahnekamp for the valuable CR 👍
I've applied all the suggestions. Now the rule is of suggestion type and fixing mechanizm for the suggestion has been enhanced (property that is being removed now includes trailing comma).

@ghost ghost requested a review from rainerhahnekamp August 11, 2024 11:54
@ghost ghost mentioned this pull request Aug 11, 2024
Closed
2 tasks
@ghost ghost requested a review from timdeschryver August 11, 2024 21:33
@ghost
Copy link
Author

ghost commented Aug 11, 2024

@rainerhahnekamp @timdeschryver converted to suggestion - ready to review

rainerhahnekamp
rainerhahnekamp reviewed Aug 11, 2024
View reviewed changes
Copy link
Contributor

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

Just left a comment on the messages. Tried it out already out in my project and confirm it works.

modules/eslint-plugin/src/rules/signals/signal-store-should-prefer-protected-state.ts Outdated Show resolved Hide resolved
modules/eslint-plugin/src/rules/signals/signal-store-should-prefer-protected-state.ts Outdated Show resolved Hide resolved
@NgDaddy and others added 2 commits August 12, 2024 01:51
@rainerhahnekamp
refactor(eslint-plugin): improve rule messge
cceb6a6 
Co-authored-by: Rainer Hahnekamp <rainer.hahnekamp@gmail.com>
@rainerhahnekamp
refactor(eslint-plugin): improve rule suggestion fix message
0f7453d 
Co-authored-by: Rainer Hahnekamp <rainer.hahnekamp@gmail.com>
@ghost
Copy link
Author

ghost commented Aug 11, 2024

@rainerhahnekamp nice catch, the messages are better now, applied :) ready to review again

@ghost ghost requested a review from rainerhahnekamp August 11, 2024 23:55
rainerhahnekamp
rainerhahnekamp approved these changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

I have also tested it manually. Good stuff @NgDaddy

@ghost
Copy link
Author

ghost commented Aug 12, 2024

thanks @rainerhahnekamp 👍 I am open for another task if you have any :)

@ghost
Copy link
Author

ghost commented Aug 12, 2024

@timdeschryver ready to CR again :)

timdeschryver
timdeschryver requested changes Aug 12, 2024
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver 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 your work on this @NgDaddy
I think the overall structure is good, I just left a few comments on details.

modules/eslint-plugin/src/rules/signals/signal-store-should-prefer-protected-state.ts Outdated Show resolved Hide resolved
modules/eslint-plugin/spec/rules/signals/signal-store-should-prefer-protected-state.spec.ts Outdated Show resolved Hide resolved
...ects/ngrx.io/content/guide/eslint-plugin/rules/signal-store-should-prefer-protected-state.md Outdated Show resolved Hide resolved
@ghost ghost requested review from timdeschryver and rainerhahnekamp August 14, 2024 23:59
@ghost ghost changed the title feat(eslint-plugin): add signalStoreShouldPreferProtectedState rule feat(eslint-plugin): add preferProtectedState rule Aug 15, 2024
@ghost
Copy link
Author

ghost commented Aug 15, 2024

@rainerhahnekamp @timdeschryver all suggestions have been applied. Ready to CR.

timdeschryver
timdeschryver approved these changes Aug 15, 2024
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Nice work, thanks @NgDaddy
The build should be resolved with #4492, then we can merge this in 😎

@ghost
Copy link
Author

ghost commented Aug 15, 2024 •
edited by ghost
Loading

At your service guys, my pleasure 😃 I am looking forward to having another opportunity to do something for @ngrx 😎

@ghost
Copy link
Author

ghost commented Aug 15, 2024

I've just pushed small refactor 2e45c0e - standardize of namings

@markostanimirovic markostanimirovic merged commit 32c772d into ngrx:main Aug 15, 2024
5 checks passed
@markostanimirovic
Copy link
Member

Thanks @NgDaddy!

@ghost ghost deleted the eslint-plugin/signal-store-prefer-protected-state branch August 15, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New Signals rule @ngrx/prefer-protected-state
3 participants
@markostanimirovic @rainerhahnekamp @timdeschryver