-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix(jira2markdown): process windows line breaks as a Unix line breaks #28
base: master
Are you sure you want to change the base?
Conversation
7857a24
to
445b489
Compare
@catcombo: I'll test it against my production pipeline today or tomorrow. Can you sync this branch with the latest master so my tests are based on the latest commits. |
445b489
to
50029e0
Compare
50029e0
to
5ed8b03
Compare
@arctus-io Sure. Done. Branch is rebased on top of the latest master. |
jira2markdown/parser.py
Outdated
@@ -17,4 +17,5 @@ def convert(text: str, usernames: Optional[dict] = None, elements: Optional[Mark | |||
inline_markup << elements.expr(inline_markup, markup, usernames, filter(lambda e: e.is_inline_element, elements)) | |||
markup << elements.expr(inline_markup, markup, usernames, elements) | |||
|
|||
text = text.replace("\r\n", "\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@catcombo: This currently works for me in my production workflow, however I would consider accounting for individual Carriage Returns (CR) )\r
). It is not an edge case that currently breaks something in my workflow but could for someone else since it was a valid new-line on legacy Mac's.
text = text.replace("\r\n", "\n").replace("\r", "\n")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I afraid of. I so not sure about these replacements. Probably this week I'll have more time to investigate how to do it with pyparsing
. Lets wait until the end of the week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed another version with a custom token for line endings. It works but now output contains mixed line endings. This is because converter doesn't change all characters in the string, but only ones that match predefined elements. Probably, it's not very good idea, but at least it works on the tokens level, not on the entire string without taking into account the string markup. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I found how to keep existing line breaks and reuse them if possible. Now in most of the cases line breaks in the converted string will be consistent. @arctus-io can you test it on your side?
Fixes #25