-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add postfix convention for store naming rule #37
Changes from 3 commits
9cdcfd9
a0d5d22
04189ad
fb2c076
ab14246
7bc1b7a
8dce4d9
52c128e
8af6d9e
919c9dc
1058555
615f1e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,19 +7,26 @@ module.exports = { | |
type: "problem", | ||
docs: { | ||
description: | ||
"Enforce $ as a prefix for any store created by Effector methods", | ||
"Enforce $ as a prefix or postfix for any store created by Effector methods", | ||
category: "Naming", | ||
recommended: true, | ||
}, | ||
messages: { | ||
invalidName: | ||
'Store "{{ storeName }}" should be named with prefix, rename it to "${{ storeName }}"', | ||
renameStore: 'Rename "{{ storeName }}" to "${{ storeName }}"', | ||
'Store "{{ storeName }}" should be named with {{ mode }}, rename it to "{{ correctedStoreName }}"', | ||
renameStore: 'Rename "{{ storeName }}" to "{{ correctedStoreName }}"', | ||
}, | ||
schema: [], | ||
schema: [ | ||
{ | ||
"enum": ["prefix", "postfix"] | ||
} | ||
], | ||
}, | ||
create(context) { | ||
const parserServices = context.parserServices; | ||
const { parserServices, options } = context; | ||
// prefix mode is a default option | ||
const [mode = "prefix"] = options; | ||
|
||
// TypeScript-way | ||
if (parserServices.hasFullTypeInformation) { | ||
return { | ||
|
@@ -38,11 +45,24 @@ module.exports = { | |
|
||
const storeName = node.id.name; | ||
|
||
if (storeName?.startsWith("$")) { | ||
if (mode === "prefix" && storeName?.startsWith("$")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we extract this logic to some place? I see, it is used here and in the JS-case. Also, I suppose it should be used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think in this case we have to change the implementation and use shared settings. Otherwise, it wouldn’t be possible to share the mode value across several rules. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like the good idea 👍 |
||
return; | ||
} | ||
if (mode === "postfix" && storeName?.endsWith("$")) { | ||
return; | ||
} | ||
|
||
const correctedStoreName = mode === "prefix" | ||
igorkamyshev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
? `$${storeName}` | ||
: `${storeName}$`; | ||
|
||
reportStoreNameConventionViolation({ context, node, storeName }); | ||
reportStoreNameConventionViolation({ | ||
context, | ||
node, | ||
storeName, | ||
correctedStoreName, | ||
mode | ||
}); | ||
}, | ||
}; | ||
} | ||
|
@@ -74,22 +94,35 @@ module.exports = { | |
} | ||
|
||
const storeName = node.parent.id.name; | ||
if (storeName.startsWith("$")) { | ||
|
||
if (mode === "prefix" && storeName.startsWith("$")) { | ||
continue; | ||
} | ||
|
||
if (mode === "postfix" && storeName.endsWith("$")) { | ||
continue; | ||
} | ||
|
||
const correctedStoreName = mode === "prefix" | ||
? `$${storeName}` | ||
: `${storeName}$`; | ||
|
||
reportStoreNameConventionViolation({ | ||
context, | ||
node: node.parent, | ||
storeName, | ||
correctedStoreName, | ||
mode | ||
}); | ||
return; | ||
} | ||
|
||
// Store creation with .map | ||
if (node.callee?.property?.name === "map") { | ||
const objectIsEffectorStore = | ||
node.callee?.object?.name?.startsWith?.("$"); | ||
const objectIsEffectorStore = mode === "prefix" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a mention before, this logic can be extracted to a separate function, so this piece of code could be simplified. |
||
? node.callee?.object?.name?.startsWith?.("$") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion, we ought to move this check to shared because it's widely used to determine Effector-stores in JS-code without type information. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, my bad, it's already extracted. Could we use |
||
: node.callee?.object?.name?.endsWith?.("$"); | ||
|
||
if (!objectIsEffectorStore) { | ||
return; | ||
} | ||
|
@@ -101,14 +134,24 @@ module.exports = { | |
} | ||
|
||
const storeName = node.parent.id.name; | ||
if (storeName.startsWith("$")) { | ||
if (mode === "prefix" && storeName.startsWith("$")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same same. |
||
return; | ||
} | ||
|
||
if (mode === "postfix" && storeName.endsWith("$")) { | ||
return; | ||
} | ||
|
||
const correctedStoreName = mode === "prefix" | ||
? `$${storeName}` | ||
: `${storeName}$`; | ||
|
||
reportStoreNameConventionViolation({ | ||
context, | ||
node: node.parent, | ||
storeName, | ||
correctedStoreName, | ||
mode | ||
}); | ||
return; | ||
} | ||
|
@@ -125,14 +168,24 @@ module.exports = { | |
} | ||
|
||
const storeName = node.parent.id.name; | ||
if (storeName.startsWith("$")) { | ||
if (mode === "prefix" && storeName.startsWith("$")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here |
||
return; | ||
} | ||
|
||
if (mode === "postfix" && storeName.endsWith("$")) { | ||
return; | ||
} | ||
|
||
const correctedStoreName = mode === "prefix" | ||
? `$${storeName}` | ||
: `${storeName}$`; | ||
|
||
reportStoreNameConventionViolation({ | ||
context, | ||
node: node.parent, | ||
storeName, | ||
correctedStoreName, | ||
mode | ||
}); | ||
return; | ||
} | ||
|
@@ -141,19 +194,24 @@ module.exports = { | |
}, | ||
}; | ||
|
||
function reportStoreNameConventionViolation({ context, node, storeName }) { | ||
function reportStoreNameConventionViolation({ context, node, storeName, correctedStoreName, mode }) { | ||
context.report({ | ||
node, | ||
messageId: "invalidName", | ||
data: { | ||
storeName, | ||
correctedStoreName, | ||
igorkamyshev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
mode | ||
}, | ||
suggest: [ | ||
{ | ||
messageId: "renameStore", | ||
data: { storeName }, | ||
fix(fixer) { | ||
return fixer.insertTextBeforeRange(node.range, "$"); | ||
if (mode === "prefix") { | ||
return fixer.insertTextBeforeRange(node.range, "$"); | ||
} | ||
return fixer.insertTextAfterRange(node.range, "$"); | ||
}, | ||
}, | ||
], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,39 @@ | ||
# effector/enforce-store-naming-convention | ||
|
||
Enforcing naming conventions helps keep the codebase consistent, and reduces overhead when thinking about how to name a variable with store. Your stores should be distingueshed by a prefix $. For example, `$name` is a store,`name` is not. | ||
Enforcing naming conventions helps keep the codebase consistent, and reduces overhead when thinking about how to name a variable with store. Depending on the configuration your stores should be distinguished by a prefix or a postfix $. Enforces prefix convention by default. | ||
|
||
## Prefix convention | ||
When configured as: | ||
```js | ||
module.exports = { | ||
rules: { | ||
"effector/enforce-store-naming-convention": ["error", "prefix"], | ||
igorkamyshev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
}; | ||
``` | ||
Prefix convention will be enforced: | ||
```ts | ||
// 👍 nice name | ||
const $name = createStore(null); | ||
|
||
// 👎 bad name | ||
const name = createStrore(null); | ||
``` | ||
## Postfix convention | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we move documentation about shred settings to main README? It seems like general info. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think yes it's a good idea to copy this info to README, but I'd also still keep the information here since it's related to the particular rule |
||
|
||
When configured as: | ||
```js | ||
module.exports = { | ||
rules: { | ||
"effector/enforce-store-naming-convention": ["error", "postfix"], | ||
}, | ||
}; | ||
``` | ||
Postfix convention will be enforced: | ||
```ts | ||
// 👍 nice name | ||
const name$ = createStore(null); | ||
|
||
// 👎 bad name | ||
const name = createStrore(null); | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
const { RuleTester } = require("eslint"); | ||
|
||
const { readExample } = require("../../../utils/read-example"); | ||
|
||
const rule = require("../enforce-store-naming-convention"); | ||
|
||
const ruleTester = new RuleTester({ | ||
parserOptions: { | ||
ecmaVersion: 2020, | ||
sourceType: "module", | ||
}, | ||
}); | ||
|
||
const readExampleForTheRule = (name) => readExample(__dirname, name); | ||
|
||
ruleTester.run("effector/enforce-store-naming-convention.test", rule, { | ||
valid: [ | ||
"correct-store-naming.js", | ||
"correct-store-naming-from-other-package.js", | ||
"correct-store-naming-in-domain.js", | ||
"correct-examples-issue-23.js", | ||
] | ||
.map(readExampleForTheRule) | ||
.map((code) => ({ code, options: ["postfix"] })), | ||
|
||
invalid: [ | ||
// Errors | ||
...[ | ||
"incorrect-createStore.js", | ||
"incorrect-createStore-alias.js", | ||
"incorrect-createStore-prefix.js", | ||
"incorrect-restore.js", | ||
"incorrect-restore-alias.js", | ||
"incorrect-combine.js", | ||
"incorrect-combine-alias.js", | ||
"incorrect-map.js", | ||
"incorrect-createStore-domain.js", | ||
] | ||
.map(readExampleForTheRule) | ||
.map((code) => ({ | ||
code, | ||
options: ["postfix"], | ||
errors: [ | ||
{ | ||
messageId: "invalidName", | ||
type: "VariableDeclarator", | ||
}, | ||
], | ||
})), | ||
// Suggestions | ||
{ | ||
code: ` | ||
import {createStore} from 'effector'; | ||
const store = createStore(null); | ||
`, | ||
errors: [ | ||
{ | ||
messageId: "invalidName", | ||
suggestions: [ | ||
{ | ||
messageId: "renameStore", | ||
data: { storeName: "store" }, | ||
output: ` | ||
import {createStore} from 'effector'; | ||
const $store = createStore(null); | ||
`, | ||
}, | ||
], | ||
}, | ||
], | ||
}, | ||
], | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
const { RuleTester } = | ||
require("@typescript-eslint/experimental-utils").ESLintUtils; | ||
const { join } = require("path"); | ||
|
||
const { readExample } = require("../../../utils/read-example"); | ||
|
||
const rule = require("../enforce-store-naming-convention"); | ||
|
||
const ruleTester = new RuleTester({ | ||
parser: "@typescript-eslint/parser", | ||
parserOptions: { | ||
ecmaVersion: 2020, | ||
sourceType: "module", | ||
project: "./tsconfig.json", | ||
tsconfigRootDir: join(__dirname, "../.."), | ||
}, | ||
}); | ||
|
||
const readExampleForTheRule = (name) => ({ | ||
code: readExample(__dirname, name), | ||
filename: join(__dirname, "examples", name), | ||
options: ["postfix"], | ||
}); | ||
|
||
ruleTester.run("effector/enforce-store-naming-convention.ts.test", rule, { | ||
valid: ["correct-store-naming.ts"].map(readExampleForTheRule), | ||
|
||
invalid: [ | ||
// Errors | ||
...["incorrect-store-naming.ts"] | ||
.map(readExampleForTheRule) | ||
.map((result) => ({ | ||
...result, | ||
errors: [ | ||
{ | ||
messageId: "invalidName", | ||
type: "VariableDeclarator", | ||
}, | ||
], | ||
})), | ||
], | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import { createDomain } from "effector"; | ||
|
||
const domain = createDomain(); | ||
|
||
const storeOne$ = domain.createStore(null); | ||
const storeTwo$ = domain.store(null); | ||
|
||
export { storeOne$, storeTwo$ }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
"use strict"; | ||
igorkamyshev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
exports.__esModule = true; | ||
exports.mappedStore$ = exports.combinedStore$ = exports.restoredStore$ = exports.justStore$ = void 0; | ||
var effector_1 = require("effector"); | ||
// Just createStore | ||
var justStore$ = (0, effector_1.createStore)(null); | ||
exports.justStore$ = justStore$; | ||
// Restore | ||
var eventForRestore = (0, effector_1.createEvent)(); | ||
var restoredStore$ = (0, effector_1.restore)(eventForRestore, null); | ||
exports.restoredStore$ = restoredStore$; | ||
// Combine | ||
var combinedStore$ = (0, effector_1.combine)(justStore$, restoredStore$); | ||
exports.combinedStore$ = combinedStore$; | ||
// Map | ||
var mappedStore$ = combinedStore$.map(function (values) { return values.length; }); | ||
exports.mappedStore$ = mappedStore$; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import { createStore, restore, createEvent, combine } from "effector"; | ||
|
||
// Just createStore | ||
const justStore$ = createStore(null); | ||
|
||
// Restore | ||
const eventForRestore = createEvent(); | ||
const restoredStore$ = restore(eventForRestore, null); | ||
|
||
// Combine | ||
const combinedStore$ = combine(justStore$, restoredStore$); | ||
|
||
// Map | ||
const mappedStore$ = combinedStore$.map((values) => values.length); | ||
|
||
export { justStore$, restoredStore$, combinedStore$, mappedStore$ }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { | ||
createStore, | ||
restore, | ||
createEvent, | ||
combine as mergeStores, | ||
} from "effector"; | ||
|
||
// Just createStore | ||
const justStore$ = createStore(null); | ||
|
||
// Restore | ||
const eventForRestore = createEvent(); | ||
const restoredStore$ = restore(eventForRestore, null); | ||
|
||
// Combine | ||
const combinedStore = mergeStores(justStore$, restoredStore$); | ||
|
||
export { combinedStore }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May we validate input here? What if user write incorrect mode? I think, we should raise a readable exception in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this, but not sure if the message is clear 😄 Do you have something on your mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the message, but I think, we should add more information — like "the configuration of effector-plugin-eslint ..."