Skip to content

🗑 DEPRECATE: StateBase.srcCharCode, int as input to skipChars, skipCharsBack, isTerminatorChar, isLetter and isSpace #199

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

Closed
wants to merge 13 commits into from

Conversation

hukkin
Copy link
Contributor

@hukkin hukkin commented Feb 13, 2022

Closes #198

Improves performance.

Deprecates:

  • StateBase.srcCharCode
  • Inputting codepoints as type int to skipChars, skipCharsBack, isTerminatorChar, isLetter and isSpace

@codecov
Copy link

codecov bot commented Feb 13, 2022

Codecov Report

Base: 96.05% // Head: 95.77% // Decreases project coverage by -0.29% ⚠️

Coverage data is based on head (e6693e1) compared to base (34876b1).
Patch coverage: 93.64% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
- Coverage   96.05%   95.77%   -0.29%     
==========================================
  Files          62       62              
  Lines        3223     3241      +18     
==========================================
+ Hits         3096     3104       +8     
- Misses        127      137      +10     
Flag Coverage Δ
pytests 95.77% <93.64%> (-0.29%) ⬇️

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

Impacted Files Coverage Δ
markdown_it/ruler.py 87.28% <57.14%> (-2.46%) ⬇️
markdown_it/rules_inline/text.py 88.88% <66.66%> (-11.12%) ⬇️
markdown_it/rules_block/state_block.py 93.70% <80.00%> (-1.98%) ⬇️
markdown_it/rules_inline/html_inline.py 89.28% <81.81%> (-6.37%) ⬇️
markdown_it/common/utils.py 87.35% <100.00%> (+0.60%) ⬆️
markdown_it/helpers/parse_link_label.py 100.00% <100.00%> (ø)
markdown_it/parser_block.py 93.47% <100.00%> (ø)
markdown_it/rules_block/blockquote.py 100.00% <100.00%> (ø)
markdown_it/rules_block/fence.py 100.00% <100.00%> (ø)
markdown_it/rules_block/heading.py 100.00% <100.00%> (ø)
... and 17 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hukkin
Copy link
Contributor Author

hukkin commented Feb 13, 2022

With this we're very close to mistletoe's performance

---------------- benchmark 'packages': 7 tests ----------------
Name (time in ms)             Mean             StdDev          
---------------------------------------------------------------
test_mistune               70.0906 (1.0)       0.6251 (1.0)    
test_mistletoe            113.1653 (1.61)      4.9409 (7.90)   
test_markdown_it_py       124.3643 (1.77)      4.7361 (7.58)   
test_pymarkdown           362.3396 (5.17)      4.2652 (6.82)   
test_commonmark_py        419.1765 (5.98)     13.6013 (21.76)  
test_pymarkdown_extra     641.6603 (9.15)     11.7974 (18.87)  
test_panflute             670.6037 (9.57)     25.7093 (41.13)  
---------------------------------------------------------------

@hukkin hukkin changed the title 🗑 DEPRECATE: StateBase.srcCharCode, int as input to skipChars, skipCharsBack and isSpace 🗑 DEPRECATE: StateBase.srcCharCode, int as input to skipChars, skipCharsBack, isTerminatorChar and isSpace Feb 13, 2022
@hukkin hukkin changed the title 🗑 DEPRECATE: StateBase.srcCharCode, int as input to skipChars, skipCharsBack, isTerminatorChar and isSpace 🗑 DEPRECATE: StateBase.srcCharCode, int as input to skipChars, skipCharsBack, isTerminatorChar, isLetter and isSpace Feb 14, 2022
@hukkin
Copy link
Contributor Author

hukkin commented Feb 18, 2022

@chrisjsewell Regarding CI configuration: If you go to Settings -> Branches -> Status checks that are required you should be able to only require the "allgood" job and "readthedocs". That should also take care of those Python 3.6 jobs still showing up above.

@chrisjsewell
Copy link
Member

Regarding CI configuration: If you go to Settings -> Branches -> Status

Dude, I set all these GitHub actions across EBP, I know how to manage them 😜 I don't think the "all good" job is really necessary, but it's not hurting anyone 😂

@hukkin
Copy link
Contributor Author

hukkin commented Feb 18, 2022

I know how to manage them

Sorry, didn't mean to imply that you don't. Mainly just noting that this can now be configured, and trying to be helpful (I always look in Settings -> Actions first 🤦 ).

I don't think the "all good" job is really necessary,

Yeah definately not a necessity, but a nice convenience in that you no longer have to change these settings when adding py3.11 removing py3.7 etc.

@chrisjsewell
Copy link
Member

Oh yeh no worries, I'll look at this stuff soonish 👍

@chrisjsewell
Copy link
Member

If you want to fix the conflicts, then we can re-look at this cheers

@hukkin
Copy link
Contributor Author

hukkin commented Apr 15, 2022

Fixed conflicts

@hukkin
Copy link
Contributor Author

hukkin commented Jun 28, 2022

@chrisjsewell I know it's a bit rude to ask, but would you have time to give this a round of review any time soonish?

I'm asking because I'd like to port some changes from upstream and add some missing type annotations, but wouldn't want to get into a cycle of fixing merge conflicts in this or my future PRs.

@chrisjsewell
Copy link
Member

Hey @hukkin, I think to move forward on this, could you first extract a separate PR that just does all the non-controversial changes, that just internally switch comparisons from ords to chars, like:

-    if state.srcCharCode[pos] != 0x3C:  # /* < */
+    if state.src[pos] != "<":

and no changes that add warnings, or change function signatures, etc

@chrisjsewell
Copy link
Member

Superseded by f52249e

I added you as co-author, hope that's ok!

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.

Improving performance
2 participants