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

remark-lint-maximum-line-length changes #318

Closed
4 tasks done
Alexendoo opened this issue Apr 11, 2024 · 17 comments · Fixed by #319
Closed
4 tasks done

remark-lint-maximum-line-length changes #318

Alexendoo opened this issue Apr 11, 2024 · 17 comments · Fixed by #319
Labels
💪 phase/solved Post is done

Comments

@Alexendoo
Copy link

Alexendoo commented Apr 11, 2024

Initial checklist

Affected packages and versions

remark-lint-maximum-line-length 4.0.0

Link to runnable example

No response

Steps to reproduce

With the following package.json, .remarkrc:

{
  "dependencies": {
    "remark-cli": "^12.0.0",
    "remark-lint": "^10.0.0",
    "remark-lint-maximum-line-length": "^4.0.0"
  }
}
{
  "plugins": [
    ["remark-lint-maximum-line-length", 120]
  ]
}

The various samples now produce the following results when run as ./node_modules/.bin/remark -u lint -f test.md

Alternatively, the [`rust-version` field](https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field)
can be used

<!-- but not -->

Alternatively, the [`rust-version` field](https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field)
test.md
1:122 warning Unexpected `121` character line, expected at most `120` characters, remove `1` character maximum-line-length remark-lint

The code can be found
[here](https://github.com/fkohlgrueber/rust-clippy-pattern/blob/039b07ecccaf96d6aa7504f5126720d2c9cceddd/clippy_lints/src/collapsible_if.rs#L88-L163).
The pattern-based

<!-- but not -->

The code can be found
[here](https://github.com/fkohlgrueber/rust-clippy-pattern/blob/039b07ecccaf96d6aa7504f5126720d2c9cceddd/clippy_lints/src/collapsible_if.rs#L88-L163).
The
test.md
2:151 warning Unexpected `150` character line, expected at most `120` characters, remove `30` characters maximum-line-length remark-lint```

Expected behavior

Ideally links at the end of lines wouldn't be linted, including ones with trailing punctuation. These cases did not lint prior to the 4.0.0 update

Actual behavior

See output

Runtime

No response

Package manager

No response

OS

No response

Build and bundle tools

No response

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Apr 11, 2024
@Alexendoo
Copy link
Author

Relatedly, the following case doesn't lint but didn't on the previous version either. It feels like it should though

* [`box_vec`](https://rust-lang.github.io/rust-clippy/master/index.html#box_collection), [`vec_box`] and [`borrowed_box`]: add link to the documentation of `Box`
  [#6023](https://github.com/rust-lang/rust-clippy/pull/6023)

[`vec_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
[`borrowed_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box

bors added a commit to rust-lang/rust-clippy that referenced this issue Apr 11, 2024
Pin `remark-lint-maximum-line-length` version

Pins the remark versions to the ones from before the most recent set of updates which errors on [some line lengths](remarkjs/remark-lint#318) that aren't clear how to resolve

Currently blocking CI

changelog:  none
@Trott
Copy link
Contributor

Trott commented Apr 14, 2024

A similar change (likely having the same cause?) in 4.0.0 that I can't tell whether it's a bug or expected/corrected behavior:

test.md:

Use
<https://www.example.com/this/is/a/really/long/url/that/that/goes/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on>
to do the thing.

.remarkrc:

{
  "plugins": [
    ["remark-lint-maximum-line-length", 120]
  ]
}

.package.json:

{
  "dependencies": {
    "remark-cli": "^12.0.0",
    "remark-lint": "^10.0.0",
    "remark-lint-maximum-line-length": "^4.0.0"
  }
}

Running npx remark -u lint -fq test.md results in:

test.md
2:154 warning Unexpected `153` character line, expected at most `120` characters, remove `33` characters maximum-line-length remark-lint

With remark-lint-maximum-line-length@3, there is no warning.

If an empty line is added above and below line 2 or if lines 1 and 3 are removed, leaving only line 2 in the file, then there is no warning.

Regression/bug or bugfix/feature? (I think it's a regression because if a URL is longer than the line length but occurs in the middle of a paragraph, it should be OK as long as it's on a line by itself. Or at least that's my possibly-naive expectation.)

@wooorm
Copy link
Member

wooorm commented Apr 14, 2024

I’m also not really sure what should happen. I improved some cases. But markdown is hard to wrap. So definitely probably some room for improvement!

@Trott
Copy link
Contributor

Trott commented Apr 17, 2024

Here's a quirk that seems like a definite bug (but as always, maybe I'm just ignorant of some nuance).

If the line after the link contains a single word, then it is permitted:

foo
<https://www.example.com/this/is/a/really/long/url/that/that/goes/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on>
bar

But if you add a second word, the previous line with the link is flagged with a warning.

foo
<https://www.example.com/this/is/a/really/long/url/that/that/goes/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on/and/on>
bar baz

@Trott
Copy link
Contributor

Trott commented Apr 17, 2024

Here's a quirk that seems like a definite bug (but as always, maybe I'm just ignorant of some nuance).

It looks like the cause of this quirk is:

Trott added a commit to Trott/remark-lint that referenced this issue Apr 17, 2024
@Trott
Copy link
Contributor

Trott commented Apr 17, 2024

#319 is my attempt to fix this issue.

wooorm pushed a commit that referenced this issue Apr 17, 2024

This comment has been minimized.

@wooorm
Copy link
Member

wooorm commented Apr 17, 2024

@wooorm wooorm added the 💪 phase/solved Post is done label Apr 17, 2024
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Apr 17, 2024
@Trott
Copy link
Contributor

Trott commented Apr 17, 2024

I think there is a related issue with remark-lint-final-definition when dealing with GFM footnotes.

This will pass remark-lint-final-definition@3 but fail remark-lint-final-definition@4:

foo[^1]

[^1]: mercury mercury mercury mercury mercury mercury mercury mercury
    mercury mercury mercury

mercury mercury

But the fix for that rule will violate remark-lint-maximum-line-length:

foo[^1]

[^1]: mercury mercury mercury mercury mercury mercury mercury mercury mercury mercury mercury

mercury mercury

Is there another solution for formatting the markdown that I'm not considering? Or is this a problem to be fixed in remark-lint-final-definition such that the old behavior is restored? Should I put this in a new issue rather than here?

@wooorm
Copy link
Member

wooorm commented Apr 17, 2024

Huh? I think you’re confusing something.

Your first example fails now because footnotes are supported, and that last mercury mercury is a separate paragraph outside the definition

The fix is not your last example, but:

foo[^1]

mercury mercury

[^1]: mercury mercury mercury mercury mercury mercury mercury mercury
    mercury mercury mercury

Or, if you want the p in the definition:

foo[^1]

[^1]: mercury mercury mercury mercury mercury mercury mercury mercury
    mercury mercury mercury

    mercury mercury

Better yet, I would not write on the first line:

foo[^1]

[^1]:
    mercury mercury mercury mercury mercury mercury mercury mercury
    mercury mercury mercury

    mercury mercury

@Trott
Copy link
Contributor

Trott commented Apr 17, 2024

Your first example fails now because footnotes are supported, and that last mercury mercury is a separate paragraph outside the definition

Ah, thank you! So, just to make sure I'm understanding correctly: What I'm seeing is because in 3.x, footnotes were ignored, and only non-GFM standard definitions were checked. But now GFM footnotes are included and being checked. Is that correct?

@Trott
Copy link
Contributor

Trott commented Apr 17, 2024

To be clear: I know that last "mercury mercury" is a separate paragraph. That's what I want! In remark-lint-final-definition@3, that wasn't flagged, but in remark-lint-final-definition@4, it is, apparently because now GFM footnotes are treated like standard definitions and the lint rule expects them to be at the end of the document. Is that right? (Sorry to post the question multiple ways. I want to make sure I understand correctly. Thanks for your patience!)

@Trott
Copy link
Contributor

Trott commented Apr 17, 2024

All three of the example solutions you provided are failing with remark-lint-final-definition@4 for me.

The fix is not your last example, but:

foo[^1]

mercury mercury

[^1]: mercury mercury mercury mercury mercury mercury mercury mercury
    mercury mercury mercury

Here's what I'm getting with that markdown:

5:1-6:28     warning Unexpected footnote definition before last content, expected definitions after line `6` final-definition remark-lint
  [cause]:
    5:7-6:28 info    Last content defined here                                                               final-definition remark-lint

@wooorm
Copy link
Member

wooorm commented Apr 17, 2024

first 2 comments: yes!
last one: looking into it! 👀

@wooorm
Copy link
Member

wooorm commented Apr 17, 2024

ahhh, full error message is interesting!

wooorm added a commit that referenced this issue Apr 17, 2024
@wooorm
Copy link
Member

wooorm commented Apr 17, 2024

Yes, GFM footnote definitions were new. Regular definitions had no children. Footnotes do. The things inside the footnote definition were seen as content after the definition! Done in https://github.com/remarkjs/remark-lint/releases/tag/remark-lint-final-definition%404.0.1

@Trott
Copy link
Contributor

Trott commented Apr 17, 2024

Yes, GFM footnote definitions were new. Regular definitions had no children. Footnotes do. The things inside the footnote definition were seen as content after the definition! Done in https://github.com/remarkjs/remark-lint/releases/tag/remark-lint-final-definition%404.0.1

Confirmed the fix in [email protected] fixes the issue I'm seeing in my code base. Thanks for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging a pull request may close this issue.

3 participants