Add flaws check / fix to cli #1623

fiji-flo · 2020-11-03T12:48:04Z

add flaws check to validate
add flaws command

peterbe · 2020-11-03T13:29:52Z

What is the context here? When would you use this? Does it solve an issue we have?

fiji-flo · 2020-11-03T16:15:45Z

The idea was to enable editors to fix flaws from the cli and add it to the validation.

Mostly parity with the UI though.

peterbe

Works! Clearly...

You'd get the same effect/result if you visit http://localhost:3000/en-US/docs/Web/CSS/background-image#_flaws and press the "Fix fixable flaws (3)" button.

The only thing I don't like is that yarn tool flaws [slug] feels like it would show the flaws or something. So it doesn't feel super intuitively named. Perhaps yarn tool fix-flaws [slug] would make more sense.

But I welcome the functionality. Perhaps it ought to be documented too. Nit?

The other thing I'm not entirely sure about is if the slug is the best entry for this. I think I'd rather be able to do:

▶ yarn tool flaws http://localhost:3000/en-US/docs/Web/CSS/background-image

or

▶ yarn tool flaws /en-US/docs/Web/CSS/background-image

But perhaps that's just me. And perhaps it would be somewhat trivial to hack around like this:

      const { slug, locale } = args;
      const { yes } = options;
      let url;
      if (slug.includes('://') || slug.startsWith('/')) {
          if (slug.includes('://')) {
              url = new URL(slug).pathname
          } else {
            url=slug;
          }
      } else {
           url=buildURL(locale, slug)
      }
      const document = Document.findByURL(url);

fiji-flo · 2020-11-04T12:01:39Z

I'd like to add some more capabilities to that command in the future. For now I think flaws is an okay name as it also just displays flaws and fixing is (kind of) optional. I get you suggestion to make it no only accept slugs but this should then change for all commands.

I filed mdn/rari#170 to follow up on this.

- add flaws check to validate - add flaws command

peterbe · 2020-11-04T16:19:59Z

Ok. Fair enough. I think this'll come in handy but only time will tell where our actual users gravitate and we won't know what for months.
You still have my approval. Now we have to wait for @escattone

escattone

@fiji-flo I really like this! I think it needs to show a more user-friendly error message though when the user provides a slug that doesn't exist. Currently it shows Cannot destructure property 'metadata' of 'document' as it is undefined.:

rjohnson-44864:deployer rjohnson$(maws_profile)$ yarn tool validate Web/ZZZ
yarn run v1.22.5
$ node tool/cli.js validate Web/ZZZ
Cannot destructure property 'metadata' of 'document' as it is undefined.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
rjohnson-44864:deployer rjohnson$(maws_profile)$ yarn tool flaws Web/ZZZ
yarn run v1.22.5
$ node tool/cli.js flaws Web/ZZZ
Cannot destructure property 'metadata' of 'document' as it is undefined.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

escattone · 2020-11-04T22:03:21Z

tool/cli.js

+      if (okay) {
+        console.log(chalk.green("✓ All seems fine"));
+      } else {
+        console.log(chalk.red("✗ Found some issues"));


This feels redundant and a bit confusing to me (e.g., are issues different than flaws?), since before this message is shown, the user will see Found X flaws.. For example:`

rjohnson-44864:deployer rjohnson$(maws_profile)$ yarn tool validate Web yarn run v1.22.5 $ node tool/cli.js validate Web Found 4 flaws. ✗ Found some issues ✨ Done in 1.28s.

Could we simply skip this message when okay is falsy?

escattone · 2020-11-04T22:21:33Z

tool/cli.js

-      Document.validate(slug, locale);
-      console.log(chalk.green("✓ All seems fine"));
+      let okay = true;
+      const document = Document.findByURL(buildURL(locale, slug));


I think we need to show a friendly error message when the document can't be found.

escattone · 2020-11-04T22:22:07Z

tool/cli.js

+    tryOrExit(async ({ args, options }) => {
+      const { slug, locale } = args;
+      const { yes } = options;
+      const document = Document.findByURL(buildURL(locale, slug));


Likewise here, a friendly error message when the document can't be found.

escattone · 2020-11-04T22:32:03Z

tool/cli.js

+      const { run } = yes
+        ? true
+        : await prompts({
+            type: "confirm",
+            message: `Proceed fixing ${flaws} flaws?`,
+            name: "run",
+            initial: true,
+          });


This is broken when -y, --yes is used on the command line:

rjohnson-44864:deployer rjohnson$(maws_profile)$ yarn tool flaws Web --yes yarn run v1.22.5 $ node tool/cli.js flaws Web --yes Would fix (link1) broken_link /en-US/docs/Web/Apps to /en-US/docs/Web/Progressive_web_apps Would fix (link4) broken_link /en-US/docs/SVG to /en-US/docs/Web/SVG Would modify "/Users/rjohnson/repos/content/files/en-us/web/index.html" from fixable flaws. ✨ Done in 1.30s.

Maybe something like:

Suggested change

const { run } = yes

? true

: await prompts({

type: "confirm",

message: `Proceed fixing ${flaws} flaws?`,

name: "run",

initial: true,

});

const run = yes

? true

: await prompts({

type: "confirm",

message: `Proceed fixing ${flaws} flaws?`,

name: "run",

initial: true,

}).run;

fiji-flo · 2020-11-05T17:45:54Z

@escattone thanks for the feedback all should be addressed

escattone

👍 Two tiny typos to fix before you merge. Thanks @fiji-flo!

tool/cli.js

escattone · 2020-11-05T22:21:56Z

@fiji-flo If you don't mind, I'll just commit the tiny typo fixes and merge this. Thanks!

fiji-flo requested review from escattone and peterbe November 3, 2020 12:48

peterbe approved these changes Nov 3, 2020

View reviewed changes

fiji-flo force-pushed the flaws-cli branch from 01c23af to f8f24df Compare November 4, 2020 12:05

Add flaws check / fix to cli

1fcd818

- add flaws check to validate - add flaws command

fiji-flo force-pushed the flaws-cli branch from f8f24df to 1fcd818 Compare November 4, 2020 15:51

escattone suggested changes Nov 4, 2020

View reviewed changes

nicer errors

1100754

fiji-flo requested a review from escattone November 5, 2020 17:46

escattone approved these changes Nov 5, 2020

View reviewed changes

tool/cli.js Outdated Show resolved Hide resolved

tool/cli.js Outdated Show resolved Hide resolved

escattone added 2 commits November 5, 2020 14:22

fix typo tool/cli.js

cec1c71

fix another typo in tool/cli.js

3d1684c

escattone merged commit 4ac5b55 into mdn:master Nov 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flaws check / fix to cli #1623

Add flaws check / fix to cli #1623

fiji-flo commented Nov 3, 2020

peterbe commented Nov 3, 2020

fiji-flo commented Nov 3, 2020

peterbe left a comment

fiji-flo commented Nov 4, 2020

peterbe commented Nov 4, 2020

escattone left a comment

escattone Nov 4, 2020

escattone Nov 4, 2020

escattone Nov 4, 2020

escattone Nov 4, 2020

fiji-flo commented Nov 5, 2020

escattone left a comment

escattone commented Nov 5, 2020

Add flaws check / fix to cli #1623

Add flaws check / fix to cli #1623

Conversation

fiji-flo commented Nov 3, 2020

peterbe commented Nov 3, 2020

fiji-flo commented Nov 3, 2020

peterbe left a comment

Choose a reason for hiding this comment

fiji-flo commented Nov 4, 2020

peterbe commented Nov 4, 2020

escattone left a comment

Choose a reason for hiding this comment

escattone Nov 4, 2020

Choose a reason for hiding this comment

escattone Nov 4, 2020

Choose a reason for hiding this comment

escattone Nov 4, 2020

Choose a reason for hiding this comment

escattone Nov 4, 2020

Choose a reason for hiding this comment

fiji-flo commented Nov 5, 2020

escattone left a comment

Choose a reason for hiding this comment

escattone commented Nov 5, 2020