Skip to content

Document exceptions in constants module#369

Merged
gsnedders merged 2 commits into
html5lib:masterfrom
willkg:307-constants
Nov 28, 2017
Merged

Document exceptions in constants module#369
gsnedders merged 2 commits into
html5lib:masterfrom
willkg:307-constants

Conversation

@willkg

@willkg willkg commented Nov 27, 2017

Copy link
Copy Markdown
Contributor

This adds docstrings to the exceptions in the constants module for #307.

@willkg

willkg commented Nov 27, 2017

Copy link
Copy Markdown
Contributor Author

I'm not as well-versed in the html5lib module as others, so please feel free to comment on improvements that should be made to the suggested text.

@willkg willkg added this to the 1.0 milestone Nov 27, 2017
@willkg

willkg commented Nov 28, 2017

Copy link
Copy Markdown
Contributor Author

@gsnedders @jgraham Can one of you review this please?

Comment thread html5lib/constants.py Outdated


class DataLossWarning(UserWarning):
"""Raised whenever html5lib changes something in such a way that some input data is lost"""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's be more specific:

Raised when the current tree is unable to represent the input data

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change this. Thanks!

Comment thread html5lib/constants.py Outdated


class ReparseException(Exception):
"""Raised when the parser should restart parsing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should really be internal; it should never be raised through any public interface.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way the docs are structured, it's difficult to make this not show up in the docs. I'll take away the docstring, but that's how it's going to have to live for 1.0.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably just _ prefix it, I guess?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! If that's important, someone who is not me can write up an issue for making those changes and make them. If that makes it in time for 1.0, that's cool. Otherwise I'm not worrying about this.

@willkg

willkg commented Nov 28, 2017

Copy link
Copy Markdown
Contributor Author

^^^ That adjusts the docstrings for those two classes per @gsnedders' comments.

@willkg

willkg commented Nov 28, 2017

Copy link
Copy Markdown
Contributor Author

AppVeyor is failing with this:

Collecting py>=1.5.0 (from pytest)
  Could not find a version that satisfies the requirement py>=1.5.0 (from pytest) (from versions: 0.9.1, 0.9.2, 1.0.0, 1.0.1, 1.0.2, 1.1.0, 1.1.1, 1.2.0, 1.2.1, 1.3.0, 1.3.1, 1.3.2, 1.3.3, 1.3.4, 1.4.0, 1.4.1, 1.4.2, 1.4.3, 1.4.4, 1.4.5, 1.4.6, 1.4.7.dev3, 1.4.7, 1.4.8, 1.4.9, 1.4.10, 1.4.11, 1.4.12, 1.4.13, 1.4.14, 1.4.15, 1.4.16, 1.4.17, 1.4.18, 1.4.19, 1.4.20, 1.4.21, 1.4.22, 1.4.23, 1.4.24, 1.4.25, 1.4.26, 1.4.27, 1.4.28, 1.4.29, 1.4.30, 1.4.31, 1.4.32, 1.4.33, 1.4.34)
No matching distribution found for py>=1.5.0 (from pytest)

That has nothing to do with this PR, so I'm going to ignore it.

@gsnedders

Copy link
Copy Markdown
Member

We also have a Travis failure, seemingly because us testing against six==1.9 and pytest now requiring six>=1.10. But yeah, unrelated.

@gsnedders gsnedders merged commit 32713a5 into html5lib:master Nov 28, 2017
@willkg

willkg commented Nov 28, 2017

Copy link
Copy Markdown
Contributor Author

Thank you!

@willkg willkg deleted the 307-constants branch November 29, 2017 14:20
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.

2 participants