-
Notifications
You must be signed in to change notification settings - Fork 32
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
dollarmath: Add allow_blank_lines
option
#46
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
I think a maintainer (@chrisjsewell?) has to click some sort of approval button in order for CI to run, to verify that I'm not mining cryptocurrency in this PR? |
@@ -302,15 +302,14 @@ def _math_block_dollar( | |||
start = state.bMarks[nextLine] + state.tShift[nextLine] | |||
end = state.eMarks[nextLine] | |||
|
|||
if end - start < 2: |
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 think this should stay (but break not continue) to safeguard against any index errors
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 line doesn't exist any more in the javascript version, why would you want to keep it in the Python version?
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.
Because JavaScript does not act the same as Python; if you index a JavaScript array with "bad indexes" it will simply return null, whereas in python it will raise an indexerror.
There has been multiple bug fixed for this in markdown-it-py
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.
The only indexing that takes place is state.src[start:end]
, and that will never give an IndexError
. If start=end
then it will return ""
, but that's absolutely fine.
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.
@rowanc1: what can we do to progress here?
@chrisjsewell: this is what I am referring to: |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #46 +/- ##
==========================================
+ Coverage 92.84% 92.90% +0.05%
==========================================
Files 30 30
Lines 1748 1748
==========================================
+ Hits 1623 1624 +1
+ Misses 125 124 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
I assume the CI failure due to a bad interaction between click and black is bogus? Thanks for triggering the CI run. |
Can I do anything to help this to go through? |
I feel this behaviour needs to be under a options flag, (a) because it is a breaking change and (b) because
I would suggest this is not always the desired behaviour in MyST |
I think there is also a potentially critical difference between block level |
Does this mean you would advocate for the change to be reverted on the javascript side? |
Oh I'm not touching lol. Just let's add it in here as a feature flag, and I think that's a good compromise |
The semantics of
I agree; but on the other hand, it seems more valuable to match what the consensus seems to be among major implementations, rather than extrapolate from a spec that doesn't actually specify the behavior here! Can I convince you of a feature flag where the default is the new behavior, or is that going to break too many things? |
Yeh just to much possibility for breakage for the outset. Of course once it is in there then it's easy to control, if defaults need to be changed |
Alright, I've added the flag to default to the old behavior. |
for more information, see https://pre-commit.ci
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.
Cheers Eric
allow_blank_lines
option
Closes #45 behind a non-default
allow_blank_lines=False
flag.Note this means that the default behavior of the javascript and python implementations has diverged.