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

Change treatment of number-lines directives. #5207

Merged
merged 1 commit into from
Jan 10, 2019
Merged

Conversation

leungbk
Copy link
Contributor

@leungbk leungbk commented Jan 9, 2019

Directives of this type without numeric inputs should not have a
startFrom attribute; with a blank value, the writers can produce
extra whitespace.

Also, the stuff around line 1000 is modified because previously, it failed to catch newlines.

Closes #5182.

@leungbk leungbk force-pushed the rst-whitespace branch 3 times, most recently from c36dd4e to df873ce Compare January 9, 2019 04:13
@leungbk
Copy link
Contributor Author

leungbk commented Jan 9, 2019

Hmm, I'm not sure why the CI can't load my file. I'm following the example of 3880.md.

@jgm
Copy link
Owner

jgm commented Jan 9, 2019

Hmm, I'm not sure why the CI can't load my file. I'm following the example of 3880.md.

You probably need to add it expliclitly to extra-source-files in pandoc.cabal.

@@ -505,7 +505,11 @@ includeDirective top fields body = do
let numberLines = lookup "number-lines" fields
let classes = trimr lang : ["numberLines" | isJust numberLines] ++
maybe [] words (lookup "class" fields)
let kvs = maybe [] (\n -> [("startFrom", trimr n)]) numberLines
let kvs = maybe [] (\n -> let tn = trimr n
Copy link
Owner

Choose a reason for hiding this comment

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

null tn would be better than tn == "". But as a matter of style, I think this is cleaner:

case trimr n of
    [] -> []
    xs -> [("startFrom", xs)]

Indeed, you could use a list comprehension:

let kvs = [("startFrom", xs) | x <- maybeToList (trimr <$> tn)]

but this probably isn't as clear to the reader.

Nothing -> []
Just n -> [("startFrom",trim n)]
Just n
Copy link
Owner

Choose a reason for hiding this comment

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

Similar remarks to the above.

Directives of this type without numeric inputs should not have a
`startFrom` attribute; with a blank value, the writers can produce
extra whitespace.
@leungbk
Copy link
Contributor Author

leungbk commented Jan 10, 2019

Done. I also tried to dedupe the code a bit.

@jgm
Copy link
Owner

jgm commented Jan 10, 2019

Looks good, thanks!

@jgm jgm merged commit 3597149 into jgm:master Jan 10, 2019
@leungbk leungbk deleted the rst-whitespace branch January 10, 2019 06:43
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

2 participants