Skip to content

Comments

Feat/fold html comments#34

Open
dbaynard wants to merge 2 commits intomasukomi:masterfrom
dbaynard:feat/fold-html-comments
Open

Feat/fold html comments#34
dbaynard wants to merge 2 commits intomasukomi:masterfrom
dbaynard:feat/fold-html-comments

Conversation

@dbaynard
Copy link

I've found it useful to fold commented sections of text.

There are two commits: the first implements the changes by duplicating the code block treatment, the second uses a pair of regexes which match both code blocks and html comments. It seems to work (so far) in nested mode, though I haven't tried stacked mode. I don't know how nested code blocks within html comments will work (or vice versa, I suppose).

I hope you find it useful, too!

@masukomi
Copy link
Owner

masukomi commented Apr 9, 2019

Sorry i didn't see this sooner @dbaynard
Sounds like a great addition. I'll look at it later today.

@masukomi
Copy link
Owner

masukomi commented Apr 9, 2019

looks good seems to work @dbaynard . I'd like to request the following changes to the PR before I merge it though

  1. can you please rebase the 2 commits into 1. It seems like you took one approach and then decided on a shorter version thus making the first commit a point in history that never really existed if we ever need to roll stuff back.
  2. can you copy the 'LineIsFenced()' tests and make a LineIsHTMLComment() set of tests?

The only differences I can think of is that the comment tests should account for

  • end lines that include text before the ---> E.g. and my comment ends on this line -->
  • end lines that include whitespace before the -->
  • end lines that start with -->
  • start lines that include trailing text after <!-- E.g. <!-- this comment starts here
  • start lines with just the <!-- (and no trailing space)
  • start lines with <!-- (including trailing space)
  • make sure it doesn't blow up on single line comments <!-- I'm a comment -->

The reason I'm asking for this is that I want to make sure that new features don't break your work without anyone noticing. I'm happy to report that your changes didn't cause the fenced code tests to fail. :)

I have added instructions for running the tests to the README. Please let me know if they are insufficient or if you have problems making it work.

@dbaynard
Copy link
Author

dbaynard commented Apr 9, 2019

Thanks for the review. I've encountered numerous corner cases myself, too. I'll run the tests — though it will likely be another week or so. I'll get back in touch, then.

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