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

Respect indent when parsing Org bullet lists #1680

Merged
merged 2 commits into from Oct 18, 2014
Merged

Conversation

thumphries
Copy link
Contributor

Fixes issue with top-level bullet list parsing in Org reader. Indent level
now matters.

Previously we would use many1 spaceChars rather than respecting
the list's indent level. We also permitted * bullets on unindented
lists, which should unambiguously parse as header 1.
Combined, this meant headers at 0-indent were
being unwittingly slurped into unambiguous preceding bullet lists, as per
Issue #1650.

Fixes issue with top-level bullet list parsing.
Previously we would use `many1 spaceChars` rather than respecting
the list's indent level. We also permitted `*` bullets on unindented
lists, which should unambiguously parse as `header 1`.
Combined, this meant headers at a different indent level were
being unwittingly slurped into preceding bullet lists, as per
Issue jgm#1650.
@tarleb
Copy link
Collaborator

tarleb commented Oct 14, 2014

Good catch and nice patch, thanks. I didn't get to test it yet, but it looks good. Some minor nitpicking: List starts are now parsed with multiple functions, allowed bullet-point characters are defined in three places. This isn't a big deal here, but it breaks the “single point of truth” principle. While being more verbose, it would (IMHO) be clearer to define a named constant containing the allowed characters (and to filter them where necessary).

@thumphries
Copy link
Contributor Author

I'll try and tidy up the constants later tonight. Not sure what to do about the double-parse of list starts. My first attempt was to change the definition of 'list continuation' to include things at the same indent level, but it required more surgery than I had time for.

Tidy up fix for jgm#1650, jgm#1698 as per comments in jgm#1680.
Fix same issue for definition lists with the same method.
@tarleb
Copy link
Collaborator

tarleb commented Oct 18, 2014

@jgm: +1 from me for this PR

jgm added a commit that referenced this pull request Oct 18, 2014
Respect indent when parsing Org bullet lists
@jgm jgm merged commit 84f6b1e into jgm:master Oct 18, 2014
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.

None yet

3 participants