Add postfix convention for store naming rule #37

ilyaryabchinski · 2021-09-09T15:37:12Z

Adds a possibility to configure convention for store naming. Prefix convention will be a default.
Most new files are created for testing purposes 🙂

closes #31

igorkamyshev · 2021-09-09T15:57:52Z

Thanks for your contribution 🙏

I'm little bit busy now, so I'll review it on Monday.

ilyaryabchinski · 2021-09-09T17:41:29Z

Don't worry 👌 Have a great rest of the week.

igorkamyshev

Thanks for your contribution, one more time. It's a great help.

I found some issues in this PR. Could you fix it, please?

Also, I found a logic problem of this feature. The rule effector/no-getState in JS-case (with unavailable type information) rely on naming convention to detect store. I think, we should add a support of postfix-notation in this rule too. Can we get options of this rule from one another? Or, we can use both notations to detect stores (it can lead to false-positive triggers in some cases)?

igorkamyshev · 2021-09-10T06:15:03Z

rules/enforce-store-naming-convention/enforce-store-naming-convention.js

@@ -38,11 +45,24 @@ module.exports = {

          const storeName = node.id.name;

-          if (storeName?.startsWith("$")) {
+          if (mode === "prefix" && storeName?.startsWith("$")) {


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 effector/no-getState JS-case (it detects stores by naming convention).

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?

It seems like the good idea 👍

rules/enforce-store-naming-convention/enforce-store-naming-convention.js

igorkamyshev · 2021-09-10T06:16:47Z

rules/enforce-store-naming-convention/enforce-store-naming-convention.js

          });
          return;
        }

        // Store creation with .map
        if (node.callee?.property?.name === "map") {
-          const objectIsEffectorStore =
-            node.callee?.object?.name?.startsWith?.("$");
+          const objectIsEffectorStore = mode === "prefix"


As a mention before, this logic can be extracted to a separate function, so this piece of code could be simplified.

igorkamyshev · 2021-09-10T06:17:04Z

rules/enforce-store-naming-convention/enforce-store-naming-convention.js

@@ -101,14 +134,24 @@ module.exports = {
          }

          const storeName = node.parent.id.name;
-          if (storeName.startsWith("$")) {
+          if (mode === "prefix" && storeName.startsWith("$")) {


igorkamyshev · 2021-09-10T06:17:12Z

rules/enforce-store-naming-convention/enforce-store-naming-convention.js

@@ -125,14 +168,24 @@ module.exports = {
          }

          const storeName = node.parent.id.name;
-          if (storeName.startsWith("$")) {
+          if (mode === "prefix" && storeName.startsWith("$")) {


rules/enforce-store-naming-convention/enforce-store-naming-convention.js

rules/enforce-store-naming-convention/enforce-store-naming-convention.md

rules/enforce-store-naming-convention/postfix/examples/correct-store-naming.js

igorkamyshev · 2021-09-10T06:23:15Z

rules/enforce-store-naming-convention/enforce-store-naming-convention.js

-    const parserServices = context.parserServices;
+    const { parserServices, options } = context;
+    // prefix mode is a default option
+    const [mode = "prefix"] = options;


May we validate input here? What if user write incorrect mode? I think, we should raise a readable exception in this case.

I added this, but not sure if the message is clear 😄 Do you have something on your mind?

I like the message, but I think, we should add more information — like "the configuration of effector-plugin-eslint ..."

ilyaryabchinski · 2021-09-10T15:49:29Z

Thank you for the review!👍

I feel sick today but I will try to fix all the comments on the upcoming weekend or the latest at the beginning of the next week.

I hope it doesn't affect your plans for 1.0 version of the project 🙏

igorkamyshev · 2021-09-10T15:54:49Z

Sure, it's ok.

Ne boley 🍀

…ent rules

ilyaryabchinski · 2021-09-17T13:08:26Z

Seems like I fixed all those things you mentioned above, but now I have a couple of questions 😄 :

Do you think that we also need to throw an error in the rule effector/no-getState if the convention is configured in the wrong way? Or it's enough to throw only in the rule effector/enforce-store-naming-convention?
I noticed that now store names like $myStore$ are valid. Do you think we should cover these cases as well ?

igorkamyshev

Sorry for the delay 🥲

It's a great job, thanks! I left some minor comments. I think, it's the last iteration 🌚

igorkamyshev · 2021-09-21T04:57:58Z

rules/enforce-store-naming-convention/enforce-store-naming-convention.js

+    const { parserServices, settings } = context;
+    // prefix convention is default
+    const storeNameConvention = settings.effector?.storeNameConvention || "prefix";
+    validateStoreNameConvention(storeNameConvention);


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

igorkamyshev · 2021-09-21T04:59:14Z

rules/enforce-store-naming-convention/enforce-store-naming-convention.js

-          const objectIsEffectorStore =
-            node.callee?.object?.name?.startsWith?.("$");
+          const objectIsEffectorStore = storeNameConvention === "prefix"
+              ? node.callee?.object?.name?.startsWith?.("$")


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.

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

igorkamyshev · 2021-09-21T05:00:45Z

rules/enforce-store-naming-convention/enforce-store-naming-convention.js

        },
      },
    ],
  });
 }
+
+function validateStoreNameConvention(storeNameConvention) {


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

igorkamyshev · 2021-09-21T05:01:50Z

rules/enforce-store-naming-convention/enforce-store-naming-convention.md

 ```ts
 // 👍 nice name
 const $name = createStore(null);

 // 👎 bad name
 const name = createStrore(null);
 ```
+## Postfix convention


Shall we move documentation about shred settings to main README? It seems like general info.

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

igorkamyshev · 2021-09-21T05:03:25Z

utils/is-store-name-valid.js

@@ -0,0 +1,13 @@
+function isStoreNameValid(storeName, storeNameConvention) {


Maybe replace storeNameConvention by context? In this case, we can move validation here too, and simplify code of rules.

igorkamyshev · 2021-09-21T05:05:36Z

@ilyaryabchinski also, in my opinion, $myStore$ is a bad way to name stores.

igorkamyshev · 2021-09-24T05:27:08Z

@ilyaryabchinski I'm going to release 0.3.0 at the weekend. If you'll finish this PR in a few days, I can wait for you before it. What do you think?

ilyaryabchinski · 2021-09-24T06:49:59Z

I am gonna work on your comments this weekend, but not sure if I can finalize all the stuff by the end of the weekend. so I think it would be safer if you release 0.3.0 without these changes.

Sorry for the delays I'm having quite busy times at work due to the end of Q3 ⏳

igorkamyshev · 2021-09-24T13:18:39Z

It's okay, no worries.

ilyaryabchinski · 2021-09-25T21:38:00Z

Your vision is great, Thank you for your comments! I resolved them and added some logic to handle these kinds of edge cases. I also extracted several functions to utils so now it looks a little cleaner(well, at least to me 😄 ).

Let's see if we are finally done with this issue 😸

igorkamyshev

Good job! Thanks a lot, it'a great help 🙏

Sorry for such a huge number of comments 😢

It'll be released it 0.3.0 today later.

ilyaryabchinski · 2021-09-26T12:59:48Z

Don't worry, I understand u have some vision and ideas on the project and also it was my first experience developing custom ESLint rules, so I learned a lot. Thank you for the collaboration 😊👍

ilyaryabchinski added 3 commits September 9, 2021 18:28

add postfix mode

9cdcfd9

change docs

a0d5d22

add tests to cover postfix mode

04189ad

igorkamyshev self-requested a review September 10, 2021 06:13

igorkamyshev requested changes Sep 10, 2021

View reviewed changes

ilyaryabchinski added 5 commits September 15, 2021 09:53

validate mode and move correctedStoreName to report function

fb2c076

fix validation

ab14246

use shared settings to be able to share name convention across differ…

7bc1b7a

…ent rules

modify tests according to the new implementation

8dce4d9

modify docs and recommended config

52c128e

modify error message

8af6d9e

igorkamyshev reviewed Sep 21, 2021

View reviewed changes

ilyaryabchinski added 3 commits September 26, 2021 00:24

extract utilities

919c9dc

fix no get state rule

1058555

refactor enforce naming rule

615f1e6

igorkamyshev approved these changes Sep 26, 2021

View reviewed changes

igorkamyshev merged commit 01e7175 into effector:master Sep 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add postfix convention for store naming rule #37

Add postfix convention for store naming rule #37

ilyaryabchinski commented Sep 9, 2021

igorkamyshev commented Sep 9, 2021

ilyaryabchinski commented Sep 9, 2021

igorkamyshev left a comment

igorkamyshev Sep 10, 2021

ilyaryabchinski Sep 15, 2021

igorkamyshev Sep 16, 2021

igorkamyshev Sep 10, 2021

igorkamyshev Sep 10, 2021

igorkamyshev Sep 10, 2021

igorkamyshev Sep 10, 2021

ilyaryabchinski Sep 15, 2021

igorkamyshev Sep 16, 2021

ilyaryabchinski commented Sep 10, 2021

igorkamyshev commented Sep 10, 2021

ilyaryabchinski commented Sep 17, 2021 •

edited

igorkamyshev left a comment

igorkamyshev Sep 21, 2021

igorkamyshev Sep 21, 2021

igorkamyshev Sep 21, 2021

igorkamyshev Sep 21, 2021

igorkamyshev Sep 21, 2021

ilyaryabchinski Sep 25, 2021

igorkamyshev Sep 21, 2021

igorkamyshev commented Sep 21, 2021

igorkamyshev commented Sep 24, 2021

ilyaryabchinski commented Sep 24, 2021

igorkamyshev commented Sep 24, 2021

ilyaryabchinski commented Sep 25, 2021

igorkamyshev left a comment

ilyaryabchinski commented Sep 26, 2021

		@@ -0,0 +1,13 @@
		function isStoreNameValid(storeName, storeNameConvention) {

Add postfix convention for store naming rule #37

Add postfix convention for store naming rule #37

Conversation

ilyaryabchinski commented Sep 9, 2021

igorkamyshev commented Sep 9, 2021

ilyaryabchinski commented Sep 9, 2021

igorkamyshev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyaryabchinski commented Sep 10, 2021

igorkamyshev commented Sep 10, 2021

ilyaryabchinski commented Sep 17, 2021 • edited

igorkamyshev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorkamyshev commented Sep 21, 2021

igorkamyshev commented Sep 24, 2021

ilyaryabchinski commented Sep 24, 2021

igorkamyshev commented Sep 24, 2021

ilyaryabchinski commented Sep 25, 2021

igorkamyshev left a comment

Choose a reason for hiding this comment

ilyaryabchinski commented Sep 26, 2021

ilyaryabchinski commented Sep 17, 2021 •

edited