Wrap strings at 80 characters #715

algomaster99 · 2019-10-20T21:50:09Z

TL;DR: See summary in #715 (comment) and discussion below.

Fixes #548

For styled blocks, I have disabled eslint max-len checking because eslint was automatically converting

@media only screen and
(min-device-width: 768px) and
(max-device-width: 1024px) {
    padding: 0px 61px 0px 67px;
    max-width: auto;
  }

to

 @media only screen and (min-device-width: 768px) and (max-device-width: 1024px) {
    padding: 0px 61px 0px 67px;
    max-width: auto;
  }

For svg tag, I thought to let d attributes as it is.
In helper.test.js, eslint was not allowing to break parametes into multiple line.

shcheklein · 2019-10-21T05:07:51Z

For styled blocks, I have disabled eslint max-len checking because eslint was automatically converting

so, I can write a styled component 100 symbols long (I think it's not good). Is there an option for eslint to respect the existing formatting (it's it's below 80).

For svg tag, I thought to let d attributes as it is.

👍

In helper.test.js, eslint was not allowing to break parametes into multiple line.

could you elaborate, please? give more details?

algomaster99 · 2019-10-21T07:04:23Z

@shcheklein ESLint automatically formatted

  it(
      "Throws error if item has source: false and doesn't have children",
      () => {
        const rawData = [{ slug: 'item-name', source: false }]

        jest.doMock('../sidebar.json', () => rawData)

        expect(() => require('./helper')).toThrow(
          new Error(
            "If you set 'source' to false, you had to add at least one child"
          )
        )
      }
    )
  })

to this

it("Throws error if item has source: false and doesn't have children", () => {
      const rawData = [{ slug: 'item-name', source: false }]

      jest.doMock('../sidebar.json', () => rawData)

      expect(() => require('./helper')).toThrow(
        new Error(
          "If you set 'source' to false, you had to add at least one child"
        )
      )
    })
  })

The latter exceeds the 80 character limit where it is defined.

jorgeorpinel · 2019-10-21T15:49:10Z

Hm... The longest line there,

it("Throws error if item has source: false and doesn't have children", () => {

is exactly 80 chars long.

src/Diagram/index.js

shcheklein

see some comments from me and @jorgeorpinel

shcheklein · 2019-10-25T18:17:07Z

@algomaster99 any progress on this? do you need any help?

algomaster99 · 2019-10-25T18:31:59Z

@shcheklein #715 (comment) see this

jorgeorpinel · 2019-10-25T23:21:26Z

To summarize, the conversation in #715 (review) (now resolved), Prettier has a weird behavior (introduced in prettier/prettier/pull/5979) that elongates some lines inside js template literals during formatting. But then hen ESLint reports an error in those lines...

@algomaster99 maybe report that bug on the Prettier repo?
Possible workarounds are described in Linebreaks weirdly in long ES6 template strings with expressions prettier/prettier#3368, better format template literals prettier/prettier#6192, and [formatting] Support for template-strings (ES6). prettier/prettier#6262 - have we tried them?
Aman is suggesting we use special comments for Prettier or ESLint to ignore those blocks.

I think that we should not include special ignore comments because we'll forget to remove them later. Unless, perhaps, they only affect that one specific line and we keep track of this bug on Prettier. However we may be forced to do this unless there is some way to force merging this without CI checks having passed. Is this possible @shcheklein?

shcheklein · 2019-10-25T23:35:30Z

@algomaster99 maybe report that bug on the Prettier repo?

+1

I think that we should not include special ignore comments because we'll forget to remove them later.

+1

CI checks having passed. Is this possible @shcheklein?

it's technically possible, but we are not going to do this since it will affect all other PRs after that - and the general status of the repo (README.md badge)

jorgeorpinel · 2019-10-25T23:42:05Z

Do you see any other alternatives/ workarounds @algomaster99? Not requiring prettier-ignore comments.

algomaster99 · 2019-10-26T14:16:52Z

@jorgeorpinel

Hm... The longest line there

It is actually indented.

Do you see any other alternatives/ workarounds

No, not yet. Thinking to open issues in prettier. Here it is: prettier/prettier#6716.

algomaster99 · 2019-10-26T14:40:47Z

@jorgeorpinel

Possible workarounds are described in prettier/prettier/issues/3368, prettier/prettier/issues/6192, and [[formatting] Support for template-strings (ES6). prettier/prettier#6262]
([formatting] Support for template-strings (ES6). prettier/prettier#6262) - have we tried them?

See prettier/prettier#6262 (comment).
~~Anyway, I don't think so there other workarounds than prettier-ignore 😕~~

Since we cannot stop prettier from elongating the template literal, let us use ignoreTemplateLiterals option?

algomaster99 · 2019-10-26T17:10:45Z

No, not yet. Thinking to open issues in prettier. Here it is: prettier/prettier#6716.

@jorgeorpinel @shcheklein They have replied. I say we go with eslint-disable-next-line. What do you suggest?

shcheklein · 2019-10-26T21:02:51Z

@algomaster99

So, first of all we have two different problems, right?

They have replied. I say we go with eslint-disable-next-line. What do you suggest?

Have you tried to google if there is a solution on the eslint side? It might be that we will have to got with that eslint-disable-next-line clause. Or alternatively just cut the description length.

Have we decided what do we do with styled blocks? And what the problem with them in the first place?

shcheklein · 2019-10-26T21:50:29Z

@algomaster99 please, check also the latest reply in the issue you created in the Prettier repo. There is an option to use a common eslint-prrettier config. I haven't check which of our problems would is solve, which would it ignore, etc.

Btw, eslint only checks the correctness it doesn't modify code, right? Prettier modifies it.

algomaster99 · 2019-10-26T21:53:38Z

@shcheklein

So, first of all we have two different problems, right?

Yes.

Wrap strings at 80 characters #715 (comment) the string after it is automatically formatted by prettier to be in one line with the argument () => {}
The media queries is styled divs are forcefully formatted by prettier to be in one line. For which I suggested max-len: ["error", {"code": 80, "ignoreTemplateLiterals": true}]. This will override can help with overriding prettier's behavior in a cleaner way. See this - https://eslint.org/docs/rules/max-len#options.

Have we decided what do we do with styled blocks?

What do you think of point 2 above?

Have you tried to google if there is a solution on the eslint side?

There was one option available for max-len called ignoreStrings but it can make the code uglier in few other places where strings could be formatted to 80 characters.

algomaster99 · 2019-10-26T22:00:27Z

Btw, eslint only checks the correctness it doesn't modify code, right?

Eslint is a code linter while prettier is a formatter. Eslint understands our code and throws errors which are more based on the syntax like Eslint will throw errors/warnings if there are unused imports, etc. But to answer your question, they both can modify it.

Eslint needs a flag --fix for modifying it.
Prettier doesn't need any.

You can check a more elaborate difference here - https://prettier.io/docs/en/comparison.html

shcheklein · 2019-10-26T22:04:06Z

would be great to see if there is a global eslint option to ignore it(...) lines. Otherwise, let's go with the eslint-disable-next-line flag. It's fine if we don't have to cover a lot of tests with it and there are no other options.

What do you think of point 2 above?

I'm not sure I understand how eslint's option helps overriding prettier's behavior.

Eslint needs a flag --fix for modifying it.

do we use it?

algomaster99 · 2019-10-26T22:09:51Z

do we use it?

No, pre-commit hook runs yarn lint which only checks it. There is no --fix flag.

I will update you when I progress on this issue. Will try turning off all the conflicting rules.

shcheklein · 2019-10-26T22:13:24Z

There is no --fix flag.

because eslint was automatically converting - is it a typo, then?

This will override can help with overriding prettier's behavior - still don't quite understand how then eslint's option can overrider prettier's behavior

Will try turning off all the conflicting rules.

sounds good ... I would focus though on the problems we have and cherry-pick relevant rules if possible

algomaster99 · 2019-10-27T06:39:16Z

@shcheklein sorry, my mistake. It was prettier not ESLint. 😅

algomaster99 · 2019-10-27T07:18:08Z

@shcheklein

still don't quite understand how then eslint's option can override prettier's behavior

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

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
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, we simply need to add one line in .eslintrc.json

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

Now getting back to the problem, max-len was already switched off because it knew conflicts due to styled-div and description of test case will arise. But there were some lines which could be formatted to 80 characters and ESlint was ignoring that too. For example, https://github.com/iterative/dvc.org/pull/715/files#diff-2d4c3e3ce4857c5300e16a64af06b639. This is why we needed to enable max-len.

algomaster99 · 2019-10-27T07:23:59Z

@shcheklein
So what do you suggest for the two problems above?.

shcheklein · 2019-10-27T15:11:22Z

we simply need to add one line in .eslintrc.json

it's already here, right?

So what do you suggest for the two problems above?

@algomaster99 as I mentioned your plan I will update you when I progress on this issue. Will try turning off all the conflicting rules. looks good to me.

algomaster99 · 2019-10-27T16:21:53Z

@shcheklein the rule are already off since we have already written the line plugin:prettier/recommended.

We just turned on one - max-len.

Now getting back to the problem, max-len was already switched off because it knew conflicts due to styled-div and description of test case will arise. But there were some lines which could be formatted to 80 characters and ESlint was ignoring that too. For example, https://github.com/iterative/dvc.org/pull/715/files#diff-2d4c3e3ce4857c5300e16a64af06b639. This is why we needed to enable max-len.

shcheklein · 2019-10-27T18:21:49Z

@algomaster99 I meant this line: "plugin:prettier/recommended" is already there? Was you referring to it in your we simply need to add one line in .eslintrc.json?

shcheklein · 2019-10-27T18:25:09Z

so, do we need to do more changes?

what about "/* eslint-disable max-len */"? as we discussed we don't consider this as a solution, right? Could you once again summarize the problems and solutions you want to apply to each of them and describe what is still missing?

algomaster99 · 2019-10-28T19:00:44Z

@shcheklein

Problem 1

prettier won't allow us to break the media queries in styled-components. prettier/prettier#5979

The small vertical line on the right indicates the 80 character limit

Proposed solution

Change "max-len": ["error", { "ignoreStrings": false }], to "max-len": ["error", { "ignoreStrings": false, "ignoreTemplateLiterals": true }],. ESLint simply ignore all template literals in our codebase and let prettier format it the way it they do. Since prettier is an opinionated code formatter we can't do anything about it and they don't have an option for it too. Using this method will have the media query line above 80 since prettier would have formatted the code before eslint is run according to our pre-commit hook configuration.
Use /* eslint-disable max-len */ and /* eslint-enable max-len */. This will format the code using prettier and when eslint is run, it will simply ignore the block enclosed between these comments. This method is currently used and it enforces us to have the media query line length above 80 since prettier formats it and then eslint ignores it.
Use // prettier-ignore just above those styled-components and then break it manually.

.
Doing this wouldn't throw any errors when running yarn lint. Using this method will wrap the media query line since we did it manually and // prettier-ignore will simply ignore formatting this block.

P.S. I also checked if any rule of stylelint could fix this. I couldn't find any.

Problem 2

prettier won't allow us to break the arguments of a test's function. prettier/prettier#953 prettier/prettier#6716

The small vertical line on the right indicates the 80 character limit

Proposed solution

Use // prettier-ignore and format it manually. The will wrap it and eslint won't throw any max-len errors.
Use // eslint-disable-next-line max-len. This will simple accept prettier's formatting and ignore the length of line when eslint is run..

shcheklein · 2019-10-28T19:08:47Z

@algomaster99 thanks for the great summary!

I vote for the solutions 1 and 2 for problems 1 and 2 respectively. This way we always accept the way prettier formats the code and we adjust ESLint to be compatible with it. It's also aligned with the way we already do this by applying "plugin:prettier/recommended".

algomaster99 · 2019-10-28T19:11:20Z

It's also aligned with the way we already do this by applying "plugin:prettier/recommended"

So that running eslint will show errors reported by prettier?

shcheklein · 2019-10-28T19:21:03Z

So that running eslint will show errors reported by prettier?

To be honest I'm not sure how does exactly plugin:prettier/recommended affect the behavior of these two. My point was that I would expect that at least it applies some set of config options to ESLint to make it compatible with what prettier expects/complaints about. Similar to applying "ignoreTemplateLiterals": true from the above manually, but prettier/recommended is doing this automatically. May be I'm wrong on this.

Since it's already enabled (I mean plugin:prettier/recommended) may be it would be great to understand what does it change. Does it have any effect on the CI runs. Etc.

jorgeorpinel · 2019-10-29T00:04:10Z

Will try turning off all the conflicting rules.

Did this not fix probelm 1? I also think this is the best strategy. See recommended ESLint config. Or are you saying their recommended config doesn't already disable ignoreTemplateLiterals @algomaster99?

I also vote for solution #1 here since it doesn't require maintaining several /* eslint-disable max-len */ and /* eslint-enable max-len */ comments.

For problem 2, could the assertion strings simply be re-written to be shorter so we avoid this problem? E.g. "Fails if source:false in leaf item"

Since it's already enabled (I mean plugin:prettier/recommended) may be it would be great to understand what does it change.

+1

jorgeorpinel · 2019-10-29T00:05:37Z

Oh no! I'm 10 minutes late to comment on this PR after it was merged! Took me like 15 to read everything and write that last note 😋

jorgeorpinel · 2019-10-29T03:50:11Z

src/Documentation/SidebarMenu/helper.test.js

+    // eslint-disable-next-line max-len
    it("Throws error if item has source: false and doesn't have children", () => {


I think we could've just shortened this string to something like "Throws error if source:false in leaf item".

src/Documentation/glossary.js

src/Documentation/Markdown/Markdown.js

This reverts commit 9b54919, reversing changes made to b477f06.

This reverts commit fd8fbbc.

algomaster99 requested review from jorgeorpinel and shcheklein October 20, 2019 21:50

shcheklein deployed to dvc-org-pr-715 October 20, 2019 21:50 View deployment

jorgeorpinel reviewed Oct 21, 2019

View reviewed changes

src/Diagram/index.js Outdated Show resolved Hide resolved

jorgeorpinel mentioned this pull request Oct 21, 2019

lint: wrap js strings (in src/) #548

Closed

shcheklein requested changes Oct 21, 2019

View reviewed changes

jorgeorpinel added the status: research Writing concrete steps for the issue label Oct 25, 2019

This comment has been minimized.

Sign in to view

Wrap strings at 80 characters

fd4fe9c

algomaster99 force-pushed the wrap-strings branch from eddfce4 to fd4fe9c Compare October 28, 2019 20:16

shcheklein approved these changes Oct 28, 2019

View reviewed changes

Merge branch 'master' into wrap-strings

2e4fa85

shcheklein merged commit 9b54919 into master Oct 28, 2019

jorgeorpinel reviewed Oct 29, 2019

View reviewed changes

src/Documentation/glossary.js Show resolved Hide resolved

jorgeorpinel reviewed Oct 29, 2019

View reviewed changes

src/Documentation/Markdown/Markdown.js Show resolved Hide resolved

shcheklein added a commit that referenced this pull request Oct 29, 2019

Revert "Merge pull request #715 from iterative/wrap-strings"

fd8fbbc

This reverts commit 9b54919, reversing changes made to b477f06.

algomaster99 added a commit that referenced this pull request Oct 29, 2019

Revert "Revert "Merge pull request #715 from iterative/wrap-strings""

e07fcf6

This reverts commit fd8fbbc.

algomaster99 mentioned this pull request Oct 29, 2019

Wrap strings at 80 characters #755

Merged

weekly-digest bot mentioned this pull request Nov 4, 2019

Weekly Digest (28 October, 2019 - 4 November, 2019) #773

Closed

		// eslint-disable-next-line max-len
		it("Throws error if item has source: false and doesn't have children", () => {

Wrap strings at 80 characters #715

Wrap strings at 80 characters #715

Conversation

algomaster99 commented Oct 20, 2019 • edited by jorgeorpinel Loading

shcheklein commented Oct 21, 2019

algomaster99 commented Oct 21, 2019

jorgeorpinel commented Oct 21, 2019

shcheklein left a comment

Choose a reason for hiding this comment

shcheklein commented Oct 25, 2019

algomaster99 commented Oct 25, 2019

jorgeorpinel commented Oct 25, 2019 • edited Loading

shcheklein commented Oct 25, 2019

jorgeorpinel commented Oct 25, 2019

algomaster99 commented Oct 26, 2019 • edited Loading

algomaster99 commented Oct 26, 2019 • edited Loading

algomaster99 commented Oct 26, 2019

shcheklein commented Oct 26, 2019

shcheklein commented Oct 26, 2019

algomaster99 commented Oct 26, 2019

algomaster99 commented Oct 26, 2019 • edited Loading

shcheklein commented Oct 26, 2019

algomaster99 commented Oct 26, 2019

shcheklein commented Oct 26, 2019

This comment has been minimized.

This comment has been minimized.

algomaster99 commented Oct 27, 2019

algomaster99 commented Oct 27, 2019

algomaster99 commented Oct 27, 2019

shcheklein commented Oct 27, 2019

algomaster99 commented Oct 27, 2019

shcheklein commented Oct 27, 2019

shcheklein commented Oct 27, 2019

algomaster99 commented Oct 28, 2019 • edited Loading

Problem 1

Proposed solution

Problem 2

Proposed solution

shcheklein commented Oct 28, 2019

algomaster99 commented Oct 28, 2019

shcheklein commented Oct 28, 2019

jorgeorpinel commented Oct 29, 2019

jorgeorpinel commented Oct 29, 2019

jorgeorpinel Oct 29, 2019

Choose a reason for hiding this comment

algomaster99 commented Oct 20, 2019 •

edited by jorgeorpinel

Loading

jorgeorpinel commented Oct 25, 2019 •

edited

Loading

algomaster99 commented Oct 26, 2019 •

edited

Loading

algomaster99 commented Oct 26, 2019 •

edited

Loading

algomaster99 commented Oct 26, 2019 •

edited

Loading

algomaster99 commented Oct 28, 2019 •

edited

Loading