Get rid of getPhases by gsnedders · Pull Request #272 · html5lib/html5lib-python · GitHub

With this we no longer include "process the token using the rules for" phases in the debug log.

This also needs perf review given it touches mainLoop.

(If you want to view the diff, append ?w=1 to the URL to ignore whitespace, otherwise the reindenting of the phases just dominates.)

Current coverage is 90.33%

Merging #272 into master will decrease coverage by 0.10%

@@             master       #272   diff @@
==========================================
  Files            51         51          
  Lines          6926       6904    -22   
  Methods           0          0          
  Messages          0          0          
  Branches       1332       1329     -3   
==========================================
- Hits           6264       6237    -27   
- Misses          501        506     +5   
  Partials        161        161

Powered by Codecov. Last updated by 945911b...a120557

@gsnedders Is this important? There's no explanatory issue, so I'm not sure what the value is here and whether we should spend time on it for 1.0 or not.

@willkg Reduction in complexity and potentially performance gains by reducing indirection.

Ok. I'm going to push this off until after 1.0, then.

Codecov Report

Merging #272 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #272      +/-   ##
==========================================
- Coverage   91.07%   91.03%   -0.04%     
==========================================
  Files          50       50              
  Lines        7044     7016      -28     
  Branches     1341     1337       -4     
==========================================
- Hits         6415     6387      -28     
  Misses        475      475              
  Partials      154      154

Impacted Files	Coverage Δ
html5lib/_utils.py	81.25% <ø> (-1.71%)	⬇️
html5lib/html5parser.py	98.41% <ø> (-0.03%)	⬇️
html5lib/tests/test_parser2.py	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 f4646e6...db7310d. Read the comment docs.

This added a fair bit of complexity, and notable made the Phase classes dynamically generated. However, by doing this, we no longer include "process the token using the rules for" phases in the debug log.

NB: I've no write or commit access here, but to me these changes look good.

From local checkout & review:

Tests pass
Performance impact (for parser benchmarks, since that's what this code affects): neutral (cpython 3.9.1)

Although there doesn't seem to be a performance improvement from this, in my opinion it does work towards making the code simpler and opens up future refactoring and cleanup work that is likely to improve performance in more significant ways.

@gsnedders Is there a reason to bring tokenTypes and self.debug into local method variables here?

At L225 we could compare using if self.debug and similarly at L226 we could use info = {"type": tokenTypes[type]}.

(am guessing it's possibly an artifact of some IDE-based refactoring? Hopefully a straightforward cleanup either way)

Nice :) I get some minor worries about difficult-to-debug future issues when built-in Python keywords like type get used as variables / returned as values. Removing this is a nice improvement 👍

Good catch! How did you discover this duplicate entry, out of interest?

The core of the improvement - looks good generally. What do you think about going one step further and moving this to the initialization of the HTMLParser's self.phases at L121?

This was merged as #567.

gsnedders force-pushed the constant_phases branch from 01cc4a8 to a120557 Compare July 13, 2016 22:47

gsnedders mentioned this pull request Feb 26, 2020

Experiment with Cython #445

Open

gsnedders force-pushed the constant_phases branch from a120557 to d764def Compare June 23, 2020 03:05

gsnedders added 2 commits September 27, 2020 21:43

Get rid of getPhases

c52e731

This added a fair bit of complexity, and notable made the Phase classes dynamically generated. However, by doing this, we no longer include "process the token using the rules for" phases in the debug log.

fixup! Get rid of getPhases

8cff6aa

gsnedders force-pushed the constant_phases branch from db7310d to 8cff6aa Compare September 27, 2020 20:44

gsnedders mentioned this pull request Jan 5, 2021

Compile html5lib with Cython #524

Draft

jayaddison reviewed Jan 9, 2021

View reviewed changes

ambv mentioned this pull request Mar 3, 2023

Constant phases #567

Merged

ambv closed this Feb 21, 2024

@@ @@ -201,6 +188,9 @@ def mainLoop(self): @@
                       DoctypeToken = tokenTypes["Doctype"]
                       ParseErrorToken = tokenTypes["ParseError"]
+                      type_names = {value: key for key, value in tokenTypes.items()}
+                      debug = self.debug

                               ('dataState', 'InBodyPhase', 'InBodyPhase', 'processStartTag', {'name': 'p', 'type': 'StartTag'}),
                               ('dataState', 'InBodyPhase', 'InBodyPhase', 'processCharacters', {'type': 'Characters'}),
                               ('dataState', 'InBodyPhase', 'InBodyPhase', 'processStartTag', {'name': 'script', 'type': 'StartTag'}),
-                              ('dataState', 'InBodyPhase', 'InHeadPhase', 'processStartTag', {'name': 'script', 'type': 'StartTag'}),

Get rid of getPhases by gsnedders · Pull Request #272 · html5lib/html5lib-python · GitHub

Uh oh!

Get rid of getPhases - #272

Get rid of getPhases#272
gsnedders wants to merge 2 commits into
html5lib:masterfrom
gsnedders:constant_phases

gsnedders commented Jul 13, 2016

Uh oh!

codecov-io commented Jul 13, 2016 •
edited
Loading

Uh oh!

Uh oh!

willkg commented Oct 3, 2017

Uh oh!

gsnedders commented Oct 5, 2017

Uh oh!

willkg commented Oct 5, 2017

Uh oh!

codecov-commenter commented Jun 24, 2020 •
edited
Loading

Uh oh!

Uh oh!

jayaddison left a comment

Uh oh!

jayaddison Jan 9, 2021

Uh oh!

jayaddison Jan 9, 2021

Uh oh!

jayaddison Jan 9, 2021

Uh oh!

jayaddison Jan 9, 2021

Uh oh!

ambv commented Feb 21, 2024 •
edited
Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		else:
		return type

		# pylint:enable=unused-argument


		_phases = {

Get rid of getPhases by gsnedders · Pull Request #272 · html5lib/html5lib-python · GitHub

Uh oh!

Get rid of getPhases - #272

Get rid of getPhases#272gsnedders wants to merge 2 commits intohtml5lib:masterhtml5lib/html5lib-python:masterfrom gsnedders:constant_phasesgsnedders/html5lib-python:constant_phasesCopy head branch name to clipboard

Conversation

gsnedders commented Jul 13, 2016

Uh oh!

codecov-io commented Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 90.33%

Uh oh!

willkg commented Oct 3, 2017

Uh oh!

gsnedders commented Oct 5, 2017

Uh oh!

willkg commented Oct 5, 2017

Uh oh!

codecov-commenter commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jayaddison left a comment

Choose a reason for hiding this comment

Uh oh!

jayaddison Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

jayaddison Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

jayaddison Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

jayaddison Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

ambv commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Get rid of getPhases#272
gsnedders wants to merge 2 commits into
html5lib:masterfrom
gsnedders:constant_phases

codecov-io commented Jul 13, 2016 •
edited
Loading

codecov-commenter commented Jun 24, 2020 •
edited
Loading

ambv commented Feb 21, 2024 •
edited
Loading