Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chore: Add failing test case for indent of multiline fixes #120

Closed
wants to merge 1 commit into from

Conversation

lydell
Copy link
Contributor

@lydell lydell commented Sep 7, 2019

If the replacement text of an autofix contains newlines, all lines but
the first in the replacement text will unexpectedly have 0 indentation
if a code block is indented. I guess not only the range of fixes but
also the text of fixes need to be adjusted.

If the code block is placed within a blockquote, a > character also
needs to be inserted.

This issue was found while investigating
lydell/eslint-plugin-simple-import-sort#22. That plugin fixes several lines
of imports in one go. This means that most of the imports will have too
little indentation after autofix if the code block is indented (such as
when placed in a list).

If the replacement text of an autofix contains newlines, all lines but
the first in the replacement text will unexpectedly have 0 indentation
if a code block is indented. I guess not only the _range_ of fixes but
also the _text_ of fixes need to be adjusted.

If the code block is placed within a blockquote, a `>` character also
needs to be inserted.

This issue was found while investigating
lydell/eslint-plugin-simple-import-sort#22.  That plugin several lines
of imports in one go. This means that most of the imports will have too
little indentation after autofix if the code block is indented (such as
when placed in a list).
@lydell
Copy link
Contributor Author

lydell commented Sep 7, 2019

FYI: I don't intend to try to fix this issue at the moment. Just wanted to let you know about it.

@btmills
Copy link
Member

btmills commented Oct 8, 2019

Well this is fun. Thank you @lydell for creating the tests! I've started playing with a potential solution in my issue120 branch, and I've gotten the tests passing without breaking any other tests. What do you think? You're right that it needs to adjust the text of fixes, which it wasn't doing before. That branch inserts whatever leading text was found on the first line of the code block after each newline in a multi-line fix. I can't decide if that's so simple it can't possibly work, or simple enough that it just might do the trick.

@lydell
Copy link
Contributor Author

lydell commented Oct 8, 2019

Will it work with underindented code? This one is tricky because the first line has less indentation than the second. Does it matter?

   ```js
 console.log('Hello, world!')
  console.log("Hello, \
  world!")
     console.log('Hello, world!')
   ```

Here’s a similar case:

   ```js
  console.log('Hello, world!')
 console.log("Hello, \
 world!")
     console.log('Hello, world!')
   ```

This time the first line has more indenation. We must make sure that we don’t accidentally end up with more spaces in the JS string, altering its .length.

Here are two other test cases I’d like to see:

  • CRLF tests.
  • A code block inside a blockquote inside another blockquote.

@lydell
Copy link
Contributor Author

lydell commented Oct 8, 2019

Also, what does this while loop do?

    while (startOffset > 0 && text[startOffset - 1] !== "\n") {
        startOffset--;
    }

EDIT: Scratch that. You seem to have removed it in the PR anyway.

btmills added a commit that referenced this pull request Oct 8, 2019
@btmills
Copy link
Member

btmills commented Oct 8, 2019

Take a look at #124. I added tests for underindenting and nested code blocks. The only caveat for underindenting is that it isn't preserved, though the fix is applied correctly. Example where the line was previously underindented by one level, and after the fix it matches the level of the fence. Extra indents are correctly preserved.

Good call on CRLF. I'm going to address that separately in #125 since it doesn't even require any indenting to fail. Pretty sure the cause is here:
https://github.com/eslint/eslint-plugin-markdown/blob/dc909618aa8f39e84279f5bdeb4a888d56d919b1/lib/processor.js#L180-L183

That line of code was a hack to get to the beginning of the line so I knew whether to add a blockquote character. Turns out I don't need to search backwards through the string; I can just do the math with the node's position, so it's gone in #124.

@btmills btmills closed this in fb0b5a3 Oct 22, 2019
@lydell lydell deleted the autofix-multiline-indent branch October 22, 2019 06:18
btmills added a commit that referenced this pull request May 20, 2021
Originally discovered in #120 and cherry-picked from #125.
btmills added a commit that referenced this pull request May 26, 2021
* Docs: Fix typo in comment

* Docs: Add RangeMap JSDoc property descriptions

* Fix: Use internally-calculated code block trim lengths

Future versions of the Markdown processor don't include per-line indent
information. Thankfully, we're already calculating it in
`getBlockRangeMap()` because we need it to correctly map fix ranges for
lines that are under-indented to the left of their opening code fence.
This includes the indent in the `RangeMap` for each line and uses that
in `adjustBlock()` instead.

This uncovered an incorrect assertion in one of the tests, which
combined an indented code block, a message on a line with mixed spaces
and tabs indentation, and an under-indented final line. The message
column was incorrectly asserted as being in the whitespace indentation
before the line.

* Chore: Test multline autofix with CRLF

Originally discovered in #120 and cherry-picked from #125.

* Update: Replace Markdown parser (fixes #125, fixes #186)

The previous parser, `remark-parse` v7, included a transitive dependency
on an npm package with a security vulnerability. Newer versions of
`remark-parse` are wrappers around a new underlying parser,
`mdast-util-from-markdown`, so we can use that directly.

The previous parser also failed to preserve `\r\n` line endings,
replacing them all with `\n`. The new parser correctly preserves `\r\n`
line endings, finally providing a fix for the failing test case I
cherry-picked in the previous commit. The improved behavior also
uncovered an incorrect line ending test assertion that this commit
corrects.

While this change is in theory fully compatible, containing just bug
fixes, I'm tagging it `Update:` in case there are compatibility changes
in the new parser. This is consistent with #175, which upgraded
`remark-parse` v5 to v7 in a semver-minor `Update:` change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants