Parse sub-lists indented with tabs #347

transitive-bullshit · 2018-08-08T23:32:23Z

Fixes #198.

@wooorm I have tested this change locally and it appears to work for sub-lists indented with space and tabs. With that being said, I wanted to get somebody's feedback on the minor changes before moving ahead with updating the test fixtures and adding tests to handle this use case.

See my discussion in #198 for a minimal repro case.

Thanks!

cc @sindresorhus sindresorhus/awesome-lint#29

transitive-bullshit · 2018-08-08T23:35:01Z

packages/remark-parse/lib/util/remove-indentation.js

-        padding = C_TAB;
-      } else {
-        padding = '';
-      }


This logic for adding padding in a function that is supposed to be removing padding is definitely incorrect. I've checked the failing test cases before and after this change, and this code block does more harm than good. There is only one test case whose output changes after making this change, and the previous fixture expected it to introduce a phantom \t into the output text paragraph. By removing this block, the phantom \t is removed and the test output is correct afaict.

transitive-bullshit · 2018-08-08T23:36:47Z

packages/remark-parse/lib/tokenize/list.js

@@ -467,7 +468,7 @@ function normalListItem(ctx, value, position) {
      $2 = C_SPACE + $2;
    }

-    max = $1 + repeat(C_SPACE, $2.length) + $3;
+    max = $1 + repeat(C_SPACE, Math.max($2.length, TAB_SIZE)) + $3;


This is necessary because we want to trim ensuing lines if they contain at least a \t, but if the first line doeesn't contain a \t, we won't properly trim them here.

I've looked through the changed (failing) test cases that this edit introduces, and afaict, their output fixtures were less correct before this change.

wooorm · 2018-08-19T10:56:50Z

packages/remark-parse/lib/tokenize/list.js

@@ -432,7 +432,8 @@ function normalListItem(ctx, value, position) {

  lines = value.split(C_NEWLINE);

-  trimmedLines = removeIndent(value, getIndent(max).indent).split(C_NEWLINE);
+  var maxIndent = getIndent(max);


could you move the var declaration to the top? So var maxIndent above, and maxIndent = getIndent(max) here?

wooorm · 2018-08-19T10:58:02Z

package.json

@@ -26,6 +26,7 @@
    "tape": "^4.5.1",
    "uglify-js": "^3.0.28",
    "unist-builder": "^1.0.2",
+    "unist-util-inspect": "^4.1.3",


Is this needed?

wooorm · 2018-08-19T10:58:36Z

@transitive-bullshit Nice! Could you run node script/regenerate-fixtures on this PR, so that we can discuss the changed fixtures?

transitive-bullshit · 2018-08-19T18:17:30Z

Thanks @wooorm. PR updated accordingly.

transitive-bullshit · 2018-08-20T07:31:02Z

There appears to be a lingering error that isn't solved by regenerating the fixtures, although it should be. I'm not sure what's going on in that case.

wooorm · 2018-08-20T09:20:59Z

What is happening here is that it’s trying the stringify the AST (tree a) to markdown, parsing that again, (tree b) and checking if both trees are equal. And it looks like for lists-with-code-and-rules they aren’t when only gfm is on (the default).

itaisteinherz · 2019-01-19T21:27:17Z

@transitive-bullshit Is this still blocked by anything? I'd be happy to help with getting this landed, and this also blocks sindresorhus/awesome#1394 from getting merged.

transitive-bullshit · 2019-01-22T12:42:45Z

@itaisteinherz @wooorm @sindresorhus, this isn't blocked by anything aside from remark being fairly complicated and me not having the time to push it past the finish line.

Feel free to pick up where I left off 💯 😄

damccorm · 2019-02-22T17:12:59Z

@transitive-bullshit FYI, I've been doing some digging here and have at least isolated the issue with the test cases a little bit further. Right now, when lists-with-code-and-rules.json is getting stringified, it produces:

1. bar:
[… we can ignore this stuff ...]
1. foo:

    line 1
    line 2

1. foo:
[… more stuff we can ignore …]

This middle section (line 1\n line 2) should get parsed as a code block, but instead is getting parsed as a paragraph block.

This is different from what's in lists-with-code-and-rules.text which your solution handles correctly and is explicitly denoted as a code block:

1. bar:
[… we can ignore this stuff ...]
1. foo:

   '''
    line 1
    line 2
   '''

1. foo:
[… more stuff we can ignore …]

I haven't figured out what exactly is causing this yet, but wanted to share in case other people are looking at the problem as well.

wooorm · 2020-04-19T07:47:51Z

Superseded by GH-485.

transitive-bullshit added 2 commits August 8, 2018 19:28

remark-parse parse sub-lists indented with tabs

ac7b893

remark-parse remove invalid block from remove-indentation

5fd0e66

transitive-bullshit commented Aug 8, 2018

View reviewed changes

transitive-bullshit changed the title ~~remark-parse parse sub-lists indented with tabs~~ Parse sub-lists indented with tabs Aug 8, 2018

transitive-bullshit mentioned this pull request Aug 9, 2018

Add multiple rules, update test structure, and require Node.js 8 sindresorhus/awesome-lint#29

Merged

10 tasks

fix lint

c920366

wooorm requested changes Aug 19, 2018

View reviewed changes

pr feedback; regenerate-fixtures

5c658f9

transitive-bullshit added 2 commits August 19, 2018 13:18

remove extra deep

244b10f

fix lint

3ae5e78

transitive-bullshit mentioned this pull request Sep 11, 2018

Fix support for tabbed sub-lists sindresorhus/awesome-lint#44

Closed

wooorm force-pushed the master branch from 1be95f1 to ce62e7a Compare October 21, 2018 10:26

wooorm closed this Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse sub-lists indented with tabs #347

Parse sub-lists indented with tabs #347

transitive-bullshit commented Aug 8, 2018 •

edited

Loading

transitive-bullshit Aug 8, 2018 •

edited

Loading

transitive-bullshit Aug 8, 2018 •

edited

Loading

wooorm Aug 19, 2018

wooorm Aug 19, 2018

transitive-bullshit Aug 19, 2018

wooorm commented Aug 19, 2018

transitive-bullshit commented Aug 19, 2018

transitive-bullshit commented Aug 20, 2018

wooorm commented Aug 20, 2018

itaisteinherz commented Jan 19, 2019

transitive-bullshit commented Jan 22, 2019

damccorm commented Feb 22, 2019 •

edited

Loading

wooorm commented Apr 19, 2020

Parse sub-lists indented with tabs #347

Parse sub-lists indented with tabs #347

Conversation

transitive-bullshit commented Aug 8, 2018 • edited Loading

transitive-bullshit Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

transitive-bullshit Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

wooorm Aug 19, 2018

Choose a reason for hiding this comment

wooorm Aug 19, 2018

Choose a reason for hiding this comment

transitive-bullshit Aug 19, 2018

Choose a reason for hiding this comment

wooorm commented Aug 19, 2018

transitive-bullshit commented Aug 19, 2018

transitive-bullshit commented Aug 20, 2018

wooorm commented Aug 20, 2018

itaisteinherz commented Jan 19, 2019

transitive-bullshit commented Jan 22, 2019

damccorm commented Feb 22, 2019 • edited Loading

wooorm commented Apr 19, 2020

transitive-bullshit commented Aug 8, 2018 •

edited

Loading

transitive-bullshit Aug 8, 2018 •

edited

Loading

transitive-bullshit Aug 8, 2018 •

edited

Loading

damccorm commented Feb 22, 2019 •

edited

Loading