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

✨ add bad linebreaks linter+fixer #269

Merged
merged 1 commit into from
Aug 24, 2023
Merged

✨ add bad linebreaks linter+fixer #269

merged 1 commit into from
Aug 24, 2023

Conversation

ctcpip
Copy link
Member

@ctcpip ctcpip commented Aug 22, 2023

tests would be nice, if someone wants to have a go 😃 .. otherwise I'll get to it eventually... probably...

package.json Outdated Show resolved Hide resolved
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :-) lots of empty line padding tho, makes it a bit hard to read.

scripts/bad-linebreaks.mjs Outdated Show resolved Hide resolved
scripts/bad-linebreaks.mjs Outdated Show resolved Hide resolved
scripts/bad-linebreaks.mjs Show resolved Hide resolved
scripts/bad-linebreaks.mjs Outdated Show resolved Hide resolved
scripts/bad-linebreaks.mjs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
scripts/bad-linebreaks.mjs Outdated Show resolved Hide resolved
scripts/bad-linebreaks.mjs Show resolved Hide resolved
@acutmore
Copy link
Contributor

Thanks @ctcpip! When I was manually regex fixing this I noticed that I got false positive match on code blocks (I had copied some code that the speaker had pasted into Matrix, so the notes would have the relevant context).

Maybe we can detect "```" blocks and ignore them. Long term feels like we should use a proper markdown parser in case there are other cases?

@OnkarRuikar
Copy link

Maybe we can detect "```" blocks and ignore them. Long term feels like we should use a proper markdown parser in case there are other cases?

Just a suggestion. May be following could work:
The markdownlint plugin markdownlint-rule-search-replace has capability to narrow scopes to only code or only text. [example code]

Config provided in this discussion will work to flag the hard wrap cases. Auto fixing, for this case, would require a separate script which could get the locations from the logs.

@ctcpip
Copy link
Member Author

ctcpip commented Aug 23, 2023

Looks good :-) lots of empty line padding tho, makes it a bit hard to read.

sir, please. these are my emotional support linebreaks

@ctcpip
Copy link
Member Author

ctcpip commented Aug 23, 2023

@acutmore

When I was manually regex fixing this I noticed that I got false positive match on code blocks

ah yes...

```javascript
a = { b: x };
a = ({ b: x });
( { b: x } = a );
( ({ b: x }) = a ); // does not work
```

right now, this code will collapse the first two lines. so in this case, I think the fix to appease this linter will be:

```javascript

a = { b: x };
a = ({ b: x });
( { b: x } = a );
( ({ b: x }) = a ); // does not work
```

not ideal, but an acceptable trade-off. agree implementing this with a proper lexer would be a lot better

@ctcpip
Copy link
Member Author

ctcpip commented Aug 23, 2023

I've addressed all comments that I'm willing to address 😄

I'll follow on with a PR to fix the linting errors

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also was serious about the excessive blank line paddings :-p

package.json Show resolved Hide resolved
scripts/bad-linebreaks.mjs Outdated Show resolved Hide resolved
@ctcpip ctcpip merged commit 337ff3c into main Aug 24, 2023
0 of 2 checks passed
@ctcpip ctcpip deleted the erroneous-linebreaks branch August 24, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants