Skip to content

Conversation

@felipesere
Copy link
Contributor

No description provided.

Copy link
Owner

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

This is looking excellent — thank you! I have a few comments on cleaning this up for merge which I think should be no trouble at all.

The only blocking issue is the now-failing spec tests. The GFM spec for task list items only recognises task list items where the marker contains whitespace, x or X in the square brackets. We're now recognising tasklists with anything inside those square brackets.

Because we're not only saving additional information about the input, but also changing the interpretation of it, we'll need to use the relaxed regex only when an flag/option is specified.

@felipesere felipesere requested a review from kivikakk December 4, 2022 20:24
@kivikakk
Copy link
Owner

kivikakk commented Dec 5, 2022

Great! As soon as we have the new option, we're good to go. Did you want to do that, or would you prefer I did?

@felipesere
Copy link
Contributor Author

felipesere commented Dec 5, 2022

I totally forgot about the flag 😅

Should this be a free-standing option or somehow tied to the TaskList extension?

@kivikakk
Copy link
Owner

kivikakk commented Dec 5, 2022

Awesome, thank you so much! Free-standing is totally fine for the option.

I've added two commits — one (a5efb05) to demonstrate an issue, and one (59e2ef5) with a suggested fix. The problem is that the regex is only initialised once for the lifetime of the program, so the first time a tasklist is processed, the setting of the option at that time is then used for all subsequent invocations.

If you're happy enough with my fix, let me know, but I didn't want to dictate the answer seeing as it's your feature! Feel free to amend it or drop the commit and do it a different way if you want. :)

@felipesere
Copy link
Contributor Author

Yeah this looks brilliant 👍

@kivikakk kivikakk merged commit 536471b into kivikakk:main Dec 7, 2022
@kivikakk
Copy link
Owner

kivikakk commented Dec 7, 2022

Awesome. Thank you so much!

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.

2 participants