New: Enable autofix (--fix)

[byzyk]

Fixes #58

[tunnckoCore]

Such little code? Coool. :)

[btmills]

Wow, this is excellent, thank you! 🎉 I had no idea it could take so little code. Can you add some tests? I'm thinking two sets:

1. In tests/lib/processor.js, manually define fixes and make sure the transformed messages have the correct ranges.
2. In tests/lib/plugin.js, check the output to make sure the fixed code looks as intended.

I care a lot more about checking edge cases in the processor tests, and a couple integration tests for the plugin ought to be sufficient.

[byzyk]

Yeah, turns out that's all we need to enable the feature. Already tried this in our webpack docs and it worked as expected - fixed around 500 linter issues in under 4 seconds.

Sure thing will add some test cases, probably tomorrow. Gonna ping you once ready for review :)

[byzyk]

@btmills please have a look at the test cases I've added.

I had to move new CLIEngine part into a separate helper function since I needed to initiate CLI with fix: true to be able to test actual autofix feature. Let me know if you feel like we should move it to separate file, perhaps to /tests/utils/cli-engine.js?

[btmills]

Thanks for turning this around so quickly, @byzyk! I tried to find some edge cases, but instead of pasting code snippets here, I sent a PR with runnable tests to your branch to make it easier for you to check out what I was trying: byzyk#1

[byzyk]

Hey @btmills! Thanks again for edge test cases, I've added them to the PR. I've managed to solve all of them except this one: inside a list.

I tried to make use of block.position.indent property to adjust offset for indented blocks accordingly but failed. Seems like there is a bug in the parser as no matter how many spaces I indent the code block with it always equal to 3 for every line in the code block. At the same time it always equal to 1 for not indented blocks.

On a side note, the test passes when it's only one line in the code block, which is really weird.

Perhaps I'm missing something here, I'd appreciate if you could be of any help. I've added two test cases: inside a list single line and inside a list multi line so you could see the difference between them.

Thanks!

[byzyk]

Hey @btmills!

I've finally managed to overcome all the obstacles and have covered all edge cases you shared with me. Please have a look at the updated code and let me know if this would be good enough to proceed.

Thanks

[btmills]

Hey @byzyk! Great work covering those edge cases 👏 I'm really excited about this 🎉

Based on your comment about the indent weirdness, and building on your two list indent tests, I started digging to see what the boundaries of the indent edge cases might be, starting with the CommonMark spec. Eliding heavily, here's the relevant portions:

A fenced code block begins with a code fence, indented no more than three spaces. If the leading code fence is indented N spaces, then up to N spaces of indentation are removed from each line of the content (if present). (If a content line is not indented, it is preserved unchanged. If it is indented less than N spaces, all of the indentation is removed.)

I can see the code as it currently stands works with fenced code blocks indented by 2 spaces, so with that as a starting point and the spec as a boundary, I tried 3 other cases:

1. One-space indentation

   This is Markdown.
   
    ```js
    console.log('Hello, world!')
    console.log('Hello, world!')
    ```

2. Three-space indentation

   This is Markdown.
   
      ```js
      console.log('Hello, world!')
      console.log('Hello, world!')
      ```

3. And finally, abandoning all decency, inconsistent indentation

   This is Markdown.
   
      ```js
    console.log('Hello, world!')
     console.log('Hello, world!')
       console.log('Hello, world!')
      ```

   For reference, after parsing, the three lines above should have indentation within the code block of 0, 0, and 1 spaces due to the truncation of underindents described in the spec quote above.

In all three code blocks, position.start.column is 1 and - similar to your observation above - position.indent is an array of all 1s. I'm really not sure what to make of that. 😩

Other avenues I'm researching:

• We're a couple versions behind on the parser. Is this perhaps a bug that's been fixed?
• The AST spec used by the parser includes definitions for Position. Is it possible I'm misinterpreting the intended meaning of the properties?
• There's a utility to pull the original source code for a Markdown AST node. Does that give us the information we need?
• I'm pretty sure these problems have existed all along, but nobody really noticed if an error message was off by a character or two in strangely-indented blocks, and autofix is the first time the need for positional accuracy has exposed these issues. Do we say "weird indentation is not supported, sorry" and move on?

So that's where I'm stuck. Thoughts on this are appreciated, brilliant insights even more so! ✨

[byzyk]

Hey @btmills! Thanks for digging into it on such deep level.

I assume none of the above examples are working correctly with the current implementation, right? That's really frustrating as there is no clear definition of indent property in AST specs, so I don't think we're misinterpreting anything here.

I suggest we can open an issue in the parser, asking for clarifications from its maintainers and add a note to the readme file, something like:

This plugin supports --fix feature... blah-blah... to make it work inside lists please indent fenced code block with 2 spaces, because of [link to the parser issue here]...

And merge it as it (with readme note) just to get this going.

Let's see where it's gonna take us, perhaps we just discovered a bug in parser or we just really misunderstood something. Regardless, some help from the outside won't harm. What do you think?

[byzyk]

Hey @btmills, just a friendly ping 🙂 What's the best way to move forward with this feature in your opinion?

[btmills]

Hi @byzyk! Sorry for the delay on this. I just went back in with fresh eyes to see if I could figure anything out, and I made no more progress than either one of us did last time. With that in mind, I think your suggestion makes sense:

1. Open an issue on the parser, linking this PR.
2. Add a note to the readme to specify that we're only able to support 2-space indentation pending resolution of the newly-created issue above.
3. Merge this and release!

Feel free to go ahead and do the first two, or let me know if you'd like me to handle those!

[byzyk]

@btmills hey man! Just a quick update. I bumped the parser version to the latest one, added a note in README and created an issue in parser repo describing our case. Looking forward to hearing your feedback 🔥

[SimenB]

Any news here? 🙂

[byzyk]

@SimenB I guess maintainers still have to review changes and unfortunately we haven't been able to make the feature work for code blocks inside lists.

[btmills]

🎉 d2243ac 🦄

First off, sorry for the lack of responsiveness here. I really want to release v1.0.0, and I really want it to include autofix, but I didn't like that it could break code in certain valid, albeit rare, edge cases. I'd been coming back to this off and on and couldn't figure out a solution, and I kept putting it off until now.

I think I've finally figured out how to handle the edge cases. Please try to break it, and send a test case if you can! I did my best to comment the code to explain what's going on, but let me know if I missed anything.

I'll give this some time for people to look over then merge and publish a release candidate if all goes well.

[byzyk]

Awesome work man! This is really cool that you've managed to overcome it in the end 👍🏽

I'll try to scrap some time this week to play around it. Will keep you posted.

[byzyk]

Good work 👍🏽

[byzyk]

Hey @btmills, just wanted to give a quick update.

I removed the note about usage in lists in README. Also played a bit with new functionality and it looks pretty solid to me. That said, it's good to go from my end 👏🏼

Let me know if you need any help before we finally merge this bad boy 😃

[btmills]

Thank you for your patience iterating on this, @byzyk - merging!

[byzyk]

Thanks, @btmills! 🎉🎉🎉

[tunnckoCore]

sweet! i'll try it