Skip to content

Document exceptions in constants module #369

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 28, 2017
Merged

Document exceptions in constants module #369

merged 2 commits into from
Nov 28, 2017

Conversation

willkg
Copy link
Contributor

@willkg willkg commented Nov 27, 2017

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

@willkg
Copy link
Contributor Author

willkg commented Nov 27, 2017

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
Copy link
Contributor Author

willkg commented Nov 28, 2017

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

@@ -2939,8 +2939,14 @@


class DataLossWarning(UserWarning):
"""Raised whenever html5lib changes something in such a way that some input data is lost"""
Copy link
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
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!

pass


class ReparseException(Exception):
"""Raised when the parser should restart parsing
Copy link
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
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
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
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
Copy link
Contributor Author

willkg commented Nov 28, 2017

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

@willkg
Copy link
Contributor Author

willkg commented Nov 28, 2017

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
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
Copy link
Contributor Author

willkg commented Nov 28, 2017

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