- Notifications
You must be signed in to change notification settings - Fork294
Get rid of getPhases#272
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedJul 13, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Current coverage is90.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
|
@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-commenter commentedJun 24, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 classesdynamically generated.However, by doing this, we no longer include "process thetoken using the rules for" phases in the debug log.
db7310d to8cff6aaCompareThere 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.
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 thatis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@gsnedders Is there a reason to bringtokenTypes andself.debug into local method variables here?
AtL225 we could compare usingif self.debug and similarly atL226 we could useinfo = {"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice :) I get some minor worries about difficult-to-debug future issues when built-in Python keywords liketype get used as variables / returned as values. Removing this is a nice improvement 👍
| @@ -68,7 +68,6 @@ def test_debug_log(): | |||
| ('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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch! How did you discover this duplicate entry, out of interest?
| # pylint:enable=unused-argument | ||
| _phases = { |
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 core of the improvement - looks good generally. What do you think about going one step further and moving this to theinitialization of theHTMLParser's self.phases at L121?
ambv commentedFeb 21, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.)