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

Add postfix convention for store naming rule #37

Merged
merged 12 commits into from
Sep 26, 2021
2 changes: 1 addition & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ module.exports = {
},
configs: {
recommended: require("./config/recommended"),
},
}
};
Original file line number Diff line number Diff line change
@@ -1,25 +1,30 @@
const {
extractImportedFromEffector,
} = require("../../utils/extract-imported-from-effector");
const { isStoreNameValid } = require("../../utils/is-store-name-valid");

module.exports = {
meta: {
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 {{ storeNameConvention }}, rename it to "{{ correctedStoreName }}"',
renameStore: 'Rename "{{ storeName }}" to "{{ correctedStoreName }}"',
},
schema: [],
},
create(context) {
const parserServices = context.parserServices;
const { parserServices, settings } = context;
// prefix convention is default
const storeNameConvention = settings.effector?.storeNameConvention || "prefix";
validateStoreNameConvention(storeNameConvention);
Copy link
Member

Choose a reason for hiding this comment

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

I think, we can pass the whole context to this function for more flexible validation.


// TypeScript-way
if (parserServices.hasFullTypeInformation) {
return {
Expand All @@ -38,11 +43,16 @@ module.exports = {

const storeName = node.id.name;

if (storeName?.startsWith("$")) {
if (isStoreNameValid(storeName, storeNameConvention)) {
return;
}

reportStoreNameConventionViolation({ context, node, storeName });
reportStoreNameConventionViolation({
context,
node,
storeName,
storeNameConvention
});
},
};
}
Expand Down Expand Up @@ -74,22 +84,26 @@ module.exports = {
}

const storeName = node.parent.id.name;
if (storeName.startsWith("$")) {

if (isStoreNameValid(storeName, storeNameConvention)) {
continue;
}

reportStoreNameConventionViolation({
context,
node: node.parent,
storeName,
storeNameConvention
});
return;
}

// Store creation with .map
if (node.callee?.property?.name === "map") {
const objectIsEffectorStore =
node.callee?.object?.name?.startsWith?.("$");
const objectIsEffectorStore = storeNameConvention === "prefix"
? node.callee?.object?.name?.startsWith?.("$")
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, my bad, it's already extracted. Could we use isStoreNameValid here?

: node.callee?.object?.name?.endsWith?.("$");

if (!objectIsEffectorStore) {
return;
}
Expand All @@ -101,14 +115,17 @@ module.exports = {
}

const storeName = node.parent.id.name;
if (storeName.startsWith("$")) {

if (isStoreNameValid(storeName, storeNameConvention)) {
return;
}


reportStoreNameConventionViolation({
context,
node: node.parent,
storeName,
storeNameConvention
});
return;
}
Expand All @@ -125,14 +142,16 @@ module.exports = {
}

const storeName = node.parent.id.name;
if (storeName.startsWith("$")) {

if (isStoreNameValid(storeName, storeNameConvention)) {
return;
}

reportStoreNameConventionViolation({
context,
node: node.parent,
storeName,
storeNameConvention
});
return;
}
Expand All @@ -141,21 +160,40 @@ module.exports = {
},
};

function reportStoreNameConventionViolation({ context, node, storeName }) {
function reportStoreNameConventionViolation({ context, node, storeName, storeNameConvention }) {

const correctedStoreName = storeNameConvention === "prefix"
? `$${storeName}`
: `${storeName}$`

context.report({
node,
messageId: "invalidName",
data: {
storeName,
correctedStoreName,
igorkamyshev marked this conversation as resolved.
Show resolved Hide resolved
storeNameConvention
},
suggest: [
{
messageId: "renameStore",
data: { storeName },
fix(fixer) {
return fixer.insertTextBeforeRange(node.range, "$");
if (storeNameConvention === "prefix") {
return fixer.insertTextBeforeRange(node.range, "$");
}
return fixer.insertTextAfterRange(node.range, "$");
},
},
],
});
}

function validateStoreNameConvention(storeNameConvention) {
Copy link
Member

Choose a reason for hiding this comment

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

So, this function looks like shred validation utility, which must be used in any rule with store defining.

if (storeNameConvention !== "prefix" && storeNameConvention !== "postfix") {
throw new Error(
"Invalid Configuration of effector-plugin-eslint/enforce-store-naming-convention. The value should be equal to prefix or postfix."
);
}
}

Original file line number Diff line number Diff line change
@@ -1,11 +1,44 @@
# 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 convention will be enforced:
```ts
// 👍 nice name
const $name = createStore(null);

// 👎 bad name
const name = createStrore(null);
```
## Postfix convention
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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",
},
settings: {
effector: {
storeNameConvention: "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,84 @@
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-postfix.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,
settings: {
effector: {
storeNameConvention: "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,
settings: {
effector: {
storeNameConvention: "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,46 @@
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),
settings: {
effector: {
storeNameConvention: "postfix"
}
},
});

ruleTester.run("effector/enforce-store-naming-convention-postfix.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,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,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$ };