Dvc.org: lint: wrap js strings (in src/)

[jorgeorpinel]

UPDATE: See https://github.com/iterative/dvc.org/issues/548#issuecomment-544209275.

---

OUTDATED:

String values in JavaScript files are currently not automatically wrapped to 80 chars by the pre-commit hook that runs pretty-quick, like all other lines of code. If possible, have the linter do this. E.g.

const strVar  = "Really long string going over 80 characters. Blah blah blah blah blah blah blah"
let templateLit = `
Another long text
Including lines that go over 80 chars. Blah blah blah blah blah blah blah blah blah blah blah blah
`

This would be pretty useful to make sure the glossary.js strings are properly wrapped, for example.

[shcheklein]

@algomaster99 would be great if you can take a look.

[jorgeorpinel]

I'm unable to do this in the Prettier playground. (Wrap at 40 chars in the linked example)

[algomaster99]

@jorgeorpinel @shcheklein they haven't addressed this issue yet. https://github.com/prettier/prettier/issues/4123 I think we need to manually wrap strings. :confused:

[jorgeorpinel]

Thanks for your input @algomaster99 we'll keep this issue open linked to that one you mentioned.

If you have time, please let us know whether you can look into the first checkbox in #461

• Fix mobile view first

and/or #510

[algomaster99]

@jorgeorpinel My exams are on. I will look into it after it :smile:

[jorgeorpinel]

If you have time now to research this one @algomaster99 (or anyone out there) it would be nice, that way we can confirm or remove the issue.

Keep in mind we're also using ESlint. This rule can probably address the concern: https://eslint.org/docs/rules/max-len

[algomaster99]

@jorgeorpinel ESLint cannot fix this but it can do detect when the line goes above 80 characters. We just need to put "max-len": [ "error", { "ignoreStrings": false } ], in configuration.

[jorgeorpinel]

Yeah, boo. I tried that rule with eslink --fix src but it doesn't fix this type of problem, it seems.

Well, let's make the issue to fix all the problems it currently reports, at least, and add the ESLint rule anyway for the future. Problems:

$ eslint src                
yarn run v1.19.1
$ /.../dvc.org/node_modules/.bin/eslint src

/.../dvc.org/src/Diagram/index.js
  209:1  error  This line has a length of 83. Maximum allowed is 80  max-len

✔️ /.../dvc.org/src/Documentation/HeadInjector.js
  10:1  error  This line has a length of 101. Maximum allowed is 80  max-len
  14:1  error  This line has a length of 87. Maximum allowed is 80   max-len
  18:1  error  This line has a length of 95. Maximum allowed is 80   max-len
  22:1  error  This line has a length of 85. Maximum allowed is 80   max-len

✔️ /.../dvc.org/src/Documentation/Markdown/Markdown.js
  58:1  error  This line has a length of 349. Maximum allowed is 80  max-len

✔️ /.../dvc.org/src/Documentation/SidebarMenu/helper.test.js
  260:1  error  This line has a length of 82. Maximum allowed is 80  max-len

✔️ /.../dvc.org/src/Documentation/glossary.js
  37:1  error  This line has a length of 83. Maximum allowed is 80  max-len
  46:1  error  This line has a length of 81. Maximum allowed is 80  max-len
  71:1  error  This line has a length of 81. Maximum allowed is 80  max-len

✔️ /.../dvc.org/src/DownloadButton/index.js
  19:1  error  This line has a length of 91. Maximum allowed is 80   max-len
  23:1  error  This line has a length of 91. Maximum allowed is 80   max-len
  27:1  error  This line has a length of 97. Maximum allowed is 80   max-len
  31:1  error  This line has a length of 100. Maximum allowed is 80  max-len

/.../dvc.org/src/LandingHero/index.js
  145:1  error  This line has a length of 83. Maximum allowed is 80  max-len
  159:1  error  This line has a length of 83. Maximum allowed is 80  max-len
  270:1  error  This line has a length of 83. Maximum allowed is 80  max-len
  294:1  error  This line has a length of 83. Maximum allowed is 80  max-len

/.../dvc.org/src/Subscribe/index.js
  32:1  error  This line has a length of 83. Maximum allowed is 80  max-len
  48:1  error  This line has a length of 83. Maximum allowed is 80  max-len

✔️ /.../dvc.org/src/SubscribeForm/index.js
   8:1  error  This line has a length of 112. Maximum allowed is 80  max-len
  25:1  error  This line has a length of 116. Maximum allowed is 80  max-len

✔️ /.../dvc.org/src/Video/index.js
  78:1  error  This line has a length of 102. Maximum allowed is 80  max-len

/.../dvc.org/src/styles.js
  78:1  error  This line has a length of 83. Maximum allowed is 80  max-len

✖ 24 problems (24 errors, 0 warnings)

UPDATE: I checked with ✔️ the ones fixed so far in #715 up to my last revision.

[algomaster99]

@jorgeorpinel yeah, we have to do it manually first. Once it is fixed, eslint would make sure this error is not there.

[shcheklein]

@algomaster99 I think the PR that was merge broke the style of the dvc.org for a few hours:

we don't have a QA team and we trust that engineers do some basic testing. Please, review it again and run it locally every time before we merge or specify [WIP].

I've reverted the changes. Please create a new PR to handle this.

@jorgeorpinel thanks for noticing this!

[algomaster99]

@shcheklein
Sorry, this happened. But I assure it is a very quick fix.

This happened because of writing URLs in src/Documentation/HeadInjector.js.
I broke:

<script
  type="text/javascript"
  src="https://cdn.jsdelivr.net/npm/[email protected]/dist/cdn/docsearch.min.js"
/>

into this:

<script
  type="text/javascript"
  src={`
    https://cdn.jsdelivr.net/npm/
    [email protected]/dist/cdn/docsearch.min.js
  `}
/>

The solution could be to:

1. Make use of template literals since we already added to ignore it in .eslintrc.

<script
    type="text/javascript"
    src={`https://cdn.jsdelivr.net/npm/[email protected]/dist/cdn/docsearch.min.js`}
/>

1. Or make use of // eslint-disable-next-line exactly what we did for that long svg tag.

<script
  type="text/javascript"
  // eslint-disable-next-line max-len
  src="https://cdn.jsdelivr.net/npm/[email protected]/dist/cdn/docsearch.min.js"
/>

https://github.com/iterative/dvc.org/pull/715/files#diff-b1d807de8934c24309015d3c1ea18882R58-R59

[shcheklein]

@algomaster99 using some special syntax for every URL looks unacceptable to me. Could you please google some discussion how do people deal with this? The most interesting thing here is that prettier broke the code. This looks like a bug to me to be honest.

[algomaster99]

@shcheklein Okay, I got it. We could again edit "max-len": ["error", { "ignoreTemplateLiterals": true }], to ignore URLs too.
Something like this:

"max-len": ["error", { "ignoreTemplateLiterals": true, "ignoreUrls": true }],

[shcheklein]

k, looks reasonable

[algomaster99]

@shcheklein
Would you recommend to enclose URLs here too in string rather than template literals just to make the codebase uniform?

[shcheklein]

@algomaster99 but they _are_ templates, right?

[algomaster99]

@shcheklein Sorry, my bad! Didn't notice. :man_facepalming:

[jorgeorpinel]

Did we ever find a full reference of what exactly plugin:prettier/recommended does in the ESLint config? Just curious.

From https://github.com/iterative/dvc.org/pull/715#issuecomment-547105692.

[algomaster99]

@jorgeorpinel https://prettier.io/docs/en/integrating-with-linters.html#recommended-configuration

[shcheklein]

@algomaster99 I think it does not answer the question :) Not very helpful link - no information whatsoever.

[algomaster99]

@jorgeorpinel @shcheklein

So there are two packages installed in our project called - eslint-config-prettier and eslint-plugin-prettier.

1. eslint-config-prettier - Its purpose is to switch off all rules which may conflict with prettier configuration. https://github.com/prettier/eslint-config-prettier#special-rules
2. eslint-plugin-prettier - Whenever you run eslint, this plugin makes sure it will run prettier too. For example, let's say we leave out a semi-colon in our code. Running eslint will return the following log.
   

Now to include these two configurations silmuntaneously, we simply need to add one line in .eslintrc.json

{
  "extends": [
    "plugin:prettier/recommended"
  ]
}

[shcheklein]

@algomaster99 I would clarify though that _special_ rules set is not the full set of rules that are being applied. Probably, this is the full list https://github.com/prettier/eslint-config-prettier/blob/master/index.js . May be there is some other logic around that changes the effective list.

[algomaster99]

@shcheklein yes, eslint-config-prettier turns off all the rules they have listen in the README . We just switched on max-len for our purpose.