Eslint plugin: Add suggestions to "data-no-store-string-literals" rule #28974
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +3.65 kB (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
david-szabo97
requested review from
ajitbohra,
nerrad and
ntwb
as code owners
|
This is awesome! I will have a closer look next week 😄 |
gziolo
reviewed
Feb 12, 2021
| function getFixes( fixer, context, callNode ) { | ||
| const storeName = callNode.arguments[ 0 ].value; | ||
| const storeDefinitions = { | ||
| core: { |
There was a problem hiding this comment.
Would it be possible to do an optimistic assumption that:
core -> @wordpress/core-data
core/(*) -> @wordpress/${1}
variable would be camelCase( '${1}Store' )
This way we won't need to keep the list up to date
There was a problem hiding this comment.
Gotcha! How I missed this 😄 5e02dff takes care of it
gziolo
reviewed
Feb 12, 2021
| { | ||
| desc: | ||
| 'Replace literal with store definition. Import store if neccessary.', | ||
| output: `import { select } from '@wordpress/data'; import { something,store as coreStore, } from '@wordpress/core-data'; select( coreStore );`, |
There was a problem hiding this comment.
The output isn't following the coding styles but I guess it's easy to fix with Prettier.
There was a problem hiding this comment.
I'd rather rely on multipass fixes 😄 Basically I apply the fix and save it immediately, thus it fixes all the auto-fixable problems. If you think it's worth fixing this I'm happy to do it though. Let me know
Source: https://eslint.org/docs/developer-guide/working-with-rules
Note: Suggestions will be applied as a stand-alone change, without triggering multipass fixes. Each suggestion should focus on a singular change in the code and should not try to conform to user defined styles. For example, if a suggestion is adding a new statement into the codebase, it should not try to match correct indentation, or confirm to user preferences on presence/absence of semicolons. All of those things can be corrected by multipass autofix when the user triggers it.
There was a problem hiding this comment.
Yes, it makes perfect sense. I considered it when mentioning Prettier, but now I see this recommendation and I totally get the idea. Even when you add spaces here, something else might add another variable to the import or reorder them and finally Prettier will do its own changes regardless.
gziolo
added
[Type] Enhancement
gziolo
approved these changes
Feb 16, 2021
There was a problem hiding this comment.
Great job, you covered all the cases I can think of right now. I love this fixer.
I only left comments related to coding styles. We prefer variable names that are optimized for reading than writing. It doesn't make much time to adjust but it's much easier to follow code with full names.
Gotcha! I had those changes lined up but forgot to commit them 😅 Pushed a commit, let's see if tests are happy. |
david-szabo97
deleted the
add/data-no-store-string-literals-fix-suggestion
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In the #28726 PR, we introduced a new rule for warning store string literals. This PR adds the functionality to replace them automatically throughout ESLint's suggestion feature.
This auto-fix only runs manually to avoid breaking Gutenberg.
This fix automatically adds the import for the store and replaces the literal with the correct name of the store. It also checks for existing store import and package imports.
There are a few cases that aren't handled:
It is limited to the hardcoded store definitions (if something is missing let me know)it's now assuming variable is always camelcase version of the store name and store name is the same as package name.coreis still hardcoded to point to@wordpress/core-dataIt helped me to replace all the warnings in
block-librarya lot faster without making any errors. Tests are also included to ensure it handles the expected cases.Examples
becomes
becomes
How has this been tested?
Screenshots
Screen.Recording.2021-02-12.at.18.04.01.mov
Types of changes
Code quality
Checklist: