GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Get rid of getPhases by gsnedders · Pull Request #272 · html5lib/html5lib-python

Conversation

gsnedders commented Jul 13, 2016

Copy link

Copy Markdown

Member

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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

codecov-io commented Jul 13, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gsnedders force-pushed the constant_phases branch from 01cc4a8 to a120557 Compare

July 13, 2016 22:47

willkg commented Oct 3, 2017

Copy link

Copy Markdown

Contributor

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gsnedders commented Oct 5, 2017

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

willkg commented Oct 5, 2017

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

codecov-commenter commented Jun 24, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

jayaddison left a comment

Copy link

Copy Markdown

Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

html5lib/html5parser.py

		@@ -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

jayaddison Jan 9, 2021

Copy link

Copy Markdown

Contributor

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 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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

html5lib/html5parser.py

		else:
		return type

jayaddison Jan 9, 2021

Copy link

Copy Markdown

Contributor

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 like type get used as variables / returned as values. Removing this is a nice improvement 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

html5lib/tests/test_parser2.py

		('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'}),

jayaddison Jan 9, 2021

Copy link

Copy Markdown

Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

html5lib/html5parser.py

		# pylint:enable=unused-argument


		_phases = {

jayaddison Jan 9, 2021

Copy link

Copy Markdown

Contributor

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 the initialization of the HTMLParser's self.phases at L121?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ambv mentioned this pull request

Mar 3, 2023

Constant phases #567

Merged

ambv commented Feb 21, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

This was merged as #567.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ambv closed this

Feb 21, 2024

Reviewers

1 more reviewer

jayaddison

jayaddison left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Jul 13, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

codecov-commenter commented Jun 24, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

ambv commented Feb 21, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.