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

🐛 FIX: Crash when file ends with empty blockquote line #186

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

hukkin
Copy link
Contributor

@hukkin hukkin commented Jan 24, 2022

Closes #185

@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #186 (8893346) into master (465b54d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #186   +/-   ##
=======================================
  Coverage   96.16%   96.16%           
=======================================
  Files          61       61           
  Lines        3259     3263    +4     
=======================================
+ Hits         3134     3138    +4     
  Misses        125      125           
Flag Coverage Δ
pytests 96.16% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
markdown_it/rules_block/blockquote.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 465b54d...8893346. Read the comment docs.

@chrisjsewell
Copy link
Member

chrisjsewell commented Jan 24, 2022

thanks @hukkin.
Given what I mentioned in #185 (comment), I'm thinking now, maybe we should just change

self.srcCharCode = tuple(ord(c) for c in self.src)

so that it sets a "defaulttuple" that returns None on out-of-bounds indexes?

@chrisjsewell
Copy link
Member

PS, ignore the docs error; that's a circular dependency issue that will be fixed when I release a new myst-nb

@hukkin
Copy link
Contributor Author

hukkin commented Jan 24, 2022

I'm thinking now, maybe we should just change ... to defaulttuple

Yeah I'd like that as well. I would suggest

  • Merge this to not delay fixing the known issue
  • Work on defaulttuple in a separate PR. We have workarounds similar to the one in this PR elsewhere in the codebase as well, so there should an attempt to remove those in the defaulttuple PR.

PS, ignore the docs error; that's a circular dependency

Yep, thanks for the heads up.

@chrisjsewell
Copy link
Member

sounds good to me 👍

@hukkin
Copy link
Contributor Author

hukkin commented Jan 24, 2022

The only issue I see with the defaulttuple approach is that if self.srcCharCode was intended as a performance hack, then moving from tuple index access to a function call may not be great for perf at all.

Something to think about / measure perf perhaps

@chrisjsewell chrisjsewell merged commit 448ea83 into executablebooks:master Jan 24, 2022
@hukkin hukkin deleted the fix-185 branch January 24, 2022 15:12
@chrisjsewell
Copy link
Member

chrisjsewell commented Jan 24, 2022

Could you not just create a simple https://docs.python.org/3/library/collections.abc.html#collections.abc.Sequence class, that has __getitem__ as a try/except on an internal tuple

@hukkin
Copy link
Contributor Author

hukkin commented Jan 24, 2022

Hmm perhaps but I wonder if you lose all C level optimization of the tuple datatype. Should probably just measure performance and see what's relevant and what's not.

@chrisjsewell
Copy link
Member

Sure ting 👍

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.

An md file, or notebook cell, ending in a blockquote with a blank line, causes a crash
2 participants