Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: Make error initializers more consistent #1655

Merged
merged 29 commits into from Feb 15, 2020
Merged

Fix: Make error initializers more consistent #1655

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
4d6873d
refactor(errors.py): unify init signature of the error classes
CaselIT Jan 25, 2020
137cee8
refactor(errors.py): remove NoRepresentation, HTTPRangeNotSatisfiable…
CaselIT Jan 25, 2020
2ac519f
refactor(errors.py): use py3 style super
CaselIT Jan 25, 2020
7bab132
test(errors): add tests to changes in errors, update other tests
CaselIT Jan 25, 2020
0ffb44b
chore(news): add news fragment for changelog
CaselIT Jan 25, 2020
c98c7f3
docs: remove references to NoRepresentation in documentation
CaselIT Jan 25, 2020
ea554a3
Merge branch 'master' into fix-777-errors
kgriffs Jan 26, 2020
8a95c65
Merge branch 'master' into fix-777-errors
vytas7 Jan 26, 2020
569cabf
refactor(errors): make the arguments kwonly
CaselIT Jan 26, 2020
6695d36
refactor(errors): ensure that the arguments of the error override the…
CaselIT Jan 26, 2020
a48e8c5
docs(errors): update error documentation adding kw arguments
CaselIT Jan 26, 2020
a55f940
refactor(error): use kw args when creating errors
CaselIT Jan 26, 2020
5e04469
chore(cython): fix errors on cython
CaselIT Jan 26, 2020
055cb04
refactor(errors): make the arguments kwonly in httperror
CaselIT Jan 26, 2020
992a2b3
docs(errors): update error documentation adding kw arguments to httpe…
CaselIT Jan 26, 2020
f3aefc4
chore(pep): fix pep errors
CaselIT Jan 26, 2020
2fef4e4
docs(news): update new for changelog
CaselIT Jan 26, 2020
c0b2ec6
fix(errors): allow a list of two tuples as the headers of the errors …
CaselIT Jan 29, 2020
af547cd
docs(error): add note about headers
CaselIT Feb 1, 2020
f1d1765
test(httperror): add missing test
CaselIT Feb 1, 2020
4109f27
Merge branch 'master' into fix-777-errors
CaselIT Feb 4, 2020
6969e0c
refactor(errors): deprecate positional arguments instead of making th…
CaselIT Feb 4, 2020
89e137a
style(pep8): fix pep8 errors
CaselIT Feb 5, 2020
e875d15
chore: add missing tests and address review
CaselIT Feb 5, 2020
ab3645a
docs: be polite
CaselIT Feb 5, 2020
c13bd66
docs(changelog): update changelog
CaselIT Feb 8, 2020
9fef3b3
docs(changelog): address review comments
CaselIT Feb 11, 2020
769de42
Merge branch 'master' into fix-777-errors
kgriffs Feb 12, 2020
8793055
Merge branch 'master' into fix-777-errors
kgriffs Feb 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions docs/_newsfragments/777-refactor-errors-kw-only.removal.rst
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions docs/_newsfragments/777-refactor-errors.feature.rst
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions docs/_newsfragments/777-refactor-errors.removal.rst
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions docs/_newsfragments/777.feature.rst
View file
Open in desktop
@@ -0,0 +1,3 @@
The error classes in ``falcon.errors`` were updated to have the ``title`` and
``description`` keyword arguments and to correctly handle headers passed as
list of tuples
4 changes: 4 additions & 0 deletions docs/_newsfragments/777.removal.rst
View file
Open in desktop
@@ -0,0 +1,4 @@
The class :class:`~.falcon.http_error.NoRepresentation` was removed since
all :class:`~.falcon.HTTPError` have optional representation.
Deprecate the use of positional arguments for the optional kw args of
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this deprecation could be its own news fragment?

Copy link
Member Author

@CaselIT CaselIT Feb 9, 2020 •
edited

Choose a reason for hiding this comment

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

I'm not sure how to name them. They should be 777 since that is the issue, and I think are both removal?

Copy link
Member

Choose a reason for hiding this comment

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

You could make the other one "misc" since it hasn't been removed (as a breaking change) yet, per se. Also, I think it is reasonable to add a few (hyphenated) words of description after the issue number (as has been done with other fragments.

@csojinb @vytas7 thoughts? Related: #1621 (comment)

Copy link
Member Author

@CaselIT CaselIT Feb 11, 2020 •
edited

Choose a reason for hiding this comment

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

I've used only the numbers as the names since I though that we wanted to use towncrier to automatically add the link to the issue for every changelog file.
If we don't need to only use the number as the name I'll make a new misc file

Copy link
Member

@kgriffs kgriffs Feb 11, 2020 •
edited

Choose a reason for hiding this comment

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

Ahh, I see. I just tried that and indeed it drops a (#{number}) reference in there, although it doesn't generate a GH link by default. That is nice, but on the other hand it seems like there should be a way to (1) have towncrier allow multiple entries for the same issue and (2) to better support items that don't have an associated issue, perhaps by simply not appending the (name-of-news-fragment). Perhaps we need a post-processing script?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a configuration parameter for towncrier to make it apply the gh link. We were trying it in gitter with @vytas7

I can just change the name for now, and then the names of the newsitems will be changed after the configuration for towncrier is figured out

Copy link
Member Author

Choose a reason for hiding this comment

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

the config was issue_format = "https://github.com/falconry/falcon/issues/{issue}"

the HTTPError subclasses