Uh oh!
There was an error while loading. Please reload this page.
Get rid of getPhases - #272
Conversation
Current coverage is 90.33%@@ 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
|
willkg
commented
Oct 3, 2017
@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. |
gsnedders
commented
Oct 5, 2017
@willkg Reduction in complexity and potentially performance gains by reducing indirection. |
willkg
commented
Oct 5, 2017
Ok. I'm going to push this off until after 1.0, then. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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.
db7310d to
8cff6aaCompare
jayaddison
left a comment
There was a problem hiding this comment.
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
parserbenchmarks, 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.
| @@ -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 | |||
There was a problem hiding this comment.
@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)
| else: | ||
| return type |
There was a problem hiding this comment.
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 👍
| ('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'}), |
There was a problem hiding this comment.
Good catch! How did you discover this duplicate entry, out of interest?
| # pylint:enable=unused-argument | ||
| _phases = { |
There was a problem hiding this comment.
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. |
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=1to the URL to ignore whitespace, otherwise the reindenting of the phases just dominates.)