style(spec): add out-of-line-enum rule APIC-416 #356

millotp · 2022-04-08T14:07:52Z

🧭 What and Why

🎟 JIRA Ticket: APIC-416

Add a rule to enforce out of line enum, except in servers section (not required here).
Also disable this rule for the bundled specs, as some refs are deleted.

Also include the simple rule ref-single-quote that enforce single quote on all $ref

Changes included:

Add out-of-line-enum rule
Add ref-single-quote rule
add tests
Lint common specs with cache

#357 contains the modification to the specs

🧪 Test

yarn docker build specs

netlify · 2022-04-08T14:07:59Z

✅ Deploy Preview for api-clients-automation ready!

Name	Link
🔨 Latest commit	`b2a79a7`
🔍 Latest deploy log	https://app.netlify.com/sites/api-clients-automation/deploys/62556156b50d94000836bc10
😎 Deploy Preview	https://deploy-preview-356--api-clients-automation.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

algolia-bot · 2022-04-08T14:09:09Z

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.

damcou

Looks very good, I tested it locally and it seems to work fine !

damcou · 2022-04-12T08:57:12Z

eslint/src/rules/outOfLineEnum.ts

+        }
+        if (
+          isPairWithKey(
+            node.parent.parent.parent.parent?.parent?.parent?.parent ?? null,


Is there any way to prevent that kind of path ?

it's a very specific edge case that we want to solve here, it won't be used anywhere else but if we need to we can have a function that checks N parents above, I will add it if I ever encounter a parent chain I will add it !

.eslintrc.js

eslint/src/rules/singleQuoteRef.ts

damcou · 2022-04-12T09:06:04Z

eslint/src/rules/singleQuoteRef.ts

+              yield fixer.replaceTextRange([end - 1, end], "'");
+            } else {
+              yield fixer.insertTextBeforeRange([start, start], "'");
+              yield fixer.insertTextAfterRange([end, end], "'");


I'd check if the last character is not ' , otherwise it will escape the last you add ;) . (in the case it doesn't start with a quote but it ends with it)

I'm not sure if I saw it elsewhere but we also need to make sure the string does not contain a ', if so we should fallback to double quoted

pretty sure " and ' are forbidden characters for path and yaml key, this is not a concern for this particular rule.

Ah yes this is only for refs sorry I forgot I was thinking of description/summaries

shortcuts

:very_nice:!!!

shortcuts · 2022-04-12T07:47:45Z

website/docs/specsCommands.md

+yarn specs:lint specs/ --fix 
 ```

 If you just want to check the format (not override the files), run:

 ```bash
-yarn specs:lint <client>
-yarn specs:lint search
+yarn specs:lint specs/<client> --fix
+yarn specs:lint specs/search --fix


The README does not mention the --fix, is that intended? Also, does it make sense to keep docs in a README since we have the same on the website?

shortcuts · 2022-04-12T07:51:09Z

package.json

@@ -25,8 +25,7 @@
    "release": "yarn workspace scripts createReleaseIssue",
    "scripts:lint": "eslint --ext=ts scripts/",
    "scripts:test": "yarn workspace scripts test",
-    "specs:fix": "eslint --ext=yml specs/$0 --fix",
-    "specs:lint": "eslint --ext=yml specs/$0",
+    "specs:lint": "eslint --ext=yml",


much cleaner

I think those comments were made before the last commit, do you me to get back to only one specs:lint script ?

Ah true, I liked the idea so it's closer to the base eslint usage, but that's just my opinion.

shortcuts · 2022-04-12T09:23:55Z

eslint/src/rules/outOfLineEnum.ts

+        }
+        if (
+          isPairWithKey(
+            node.parent.parent.parent.parent?.parent?.parent?.parent ?? null,


We can maybe have a function that will find the deepest parent in a given element, so it handles even deeper ones etc.

Here it's not necessarily the deepest, it's just that's the 7th parent has be to a pair with key servers, not sure if it generalizable.

Yes but there should only be one servers in a spec so we can maybe assume that the deepest or first found is correct?

we could, but that won't make the code simpler so I don't see why change

it was more to make sure if it's the 6th or 9th it won't require an other manual check, it's not necessary to change it right now, we can iterate on it if there's an issue one day

shortcuts · 2022-04-12T09:25:11Z

eslint/src/rules/singleQuoteRef.ts

+          return;
+        }
+        if (!isScalar(node.value)) {
+          // not our problem


can you elaborate why it's not?

If the ref is not a scalar (not a string) then something else will fail, like the path resolution of redocly so we don't need to check here, I will comment.

Yes I meant elaborate the comment, I get the reason :D

shortcuts · 2022-04-12T09:26:01Z

eslint/src/rules/singleQuoteRef.ts

+          return;
+        }
+        if (isBLockScalar(node.value)) {
+          // what to do here ?


well it could maybe work with block scalar I haven't tried it, but it should be forbidden by another rule because it doesn't make sense.

yep definitely, I guess blocks can be as is it's fine

shortcuts · 2022-04-12T09:26:20Z

eslint/src/rules/singleQuoteRef.ts

+        const hasDoubleQuote = node.value.style === 'double-quoted';
+        const [start, end] = node.value.range;
+        context.report({
+          node: node as any,


how come we need to cast 🤔

There is an issue between eslint type Node and yaml-eslint-parser type that inherits from the wrong Node, so type are not compatible but in practice they have the same properties.

shortcuts · 2022-04-12T09:28:24Z

eslint/src/rules/singleQuoteRef.ts

+              yield fixer.replaceTextRange([end - 1, end], "'");
+            } else {
+              yield fixer.insertTextBeforeRange([start, start], "'");
+              yield fixer.insertTextAfterRange([end, end], "'");


I'm not sure if I saw it elsewhere but we also need to make sure the string does not contain a ', if so we should fallback to double quoted

shortcuts · 2022-04-12T09:30:54Z

scripts/buildSpecs.ts

+  const spinner = createSpinner('linting common spec', verbose).start();
+  await run(`yarn specs:lint common`, { verbose });


don't we want to fix when it's not the CI?

I'm not a fan of scripts modifying hand written file when it's unclear, but here it should be fine

Make sense but if it's only for dev purposes it's fine I guess

(as you want)

shortcuts

just so we remove bundled specs from the PR

shortcuts · 2022-04-12T10:00:20Z

specs/bundled/analytics.yml

@@ -53,7 +53,7 @@ components:
        period to analyze.


bundled specs changes are pushed by the CI

ah yes sorry I forgot

…tomation into style/eslint-enum

shortcuts

Pretty solid! Good addition ✍🏼

millotp self-assigned this Apr 8, 2022

millotp requested review from a team, damcou and shortcuts and removed request for a team April 8, 2022 14:10

millotp force-pushed the style/eslint-enum branch from 87040bd to 9d77035 Compare April 8, 2022 15:14

style(spec): add out-of-line-enum rule

2d36490

millotp force-pushed the style/eslint-enum branch from 9d77035 to 2d36490 Compare April 12, 2022 07:53

millotp added 2 commits April 12, 2022 10:36

fix(spec): apply eslint (#357)

9af3d59

Merge branch 'main' into style/eslint-enum

67d8f9d

damcou reviewed Apr 12, 2022

View reviewed changes

shortcuts reviewed Apr 12, 2022

View reviewed changes

shortcuts requested changes Apr 12, 2022

View reviewed changes

millotp added 4 commits April 12, 2022 13:21

comments

4b149ba

Merge branch 'style/eslint-enum' of github.com:algolia/api-clients-au…

5e8757f

…tomation into style/eslint-enum

remove bundled

05a1a0e

review

b2a79a7

millotp requested a review from shortcuts April 12, 2022 11:25

shortcuts approved these changes Apr 12, 2022

View reviewed changes

millotp merged commit 5fd1391 into main Apr 12, 2022

millotp deleted the style/eslint-enum branch April 12, 2022 13:09

shortcuts pushed a commit that referenced this pull request Apr 22, 2022

style(spec): add out-of-line-enum rule APIC-416 (#356)

b546a32

shortcuts mentioned this pull request May 25, 2022

chore: prepare release 2022-05-25 #549

Closed

		const spinner = createSpinner('linting common spec', verbose).start();
		await run(`yarn specs:lint common`, { verbose });

style(spec): add out-of-line-enum rule APIC-416 #356

style(spec): add out-of-line-enum rule APIC-416 #356

Conversation

millotp commented Apr 8, 2022 • edited Loading

🧭 What and Why

Changes included:

🧪 Test

netlify bot commented Apr 8, 2022 • edited Loading

✅ Deploy Preview for api-clients-automation ready!

algolia-bot commented Apr 8, 2022 • edited Loading

✗ The generated branch has been deleted.

damcou 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

shortcuts 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

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

Choose a reason for hiding this comment

shortcuts Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

shortcuts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shortcuts left a comment

Choose a reason for hiding this comment

millotp commented Apr 8, 2022 •

edited

Loading

netlify bot commented Apr 8, 2022 •

edited

Loading

algolia-bot commented Apr 8, 2022 •

edited

Loading

shortcuts Apr 12, 2022 •

edited

Loading