Fix: Make error initializers more consistent #1655

CaselIT · 2020-01-25T19:13:40Z

Summary of Changes

Normalized the interface of the http errors with common title and description kwargs.
Compared to the issue I've left the headers kw as is, since it's used in all errors

Related Issues

Fixes: #777

Pull Request Checklist

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

PR template inspired by the attrs project.

… has optional representation

codecov · 2020-01-25T19:19:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@769de42). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #1655   +/-   ##
========================================
  Coverage          ?    100%           
========================================
  Files             ?      47           
  Lines             ?    3565           
  Branches          ?     545           
========================================
  Hits              ?    3565           
  Misses            ?       0           
  Partials          ?       0

Impacted Files	Coverage Δ
falcon/constants.py	`100% <100%> (ø)`
falcon/util/misc.py	`100% <100%> (ø)`
falcon/media/__init__.py	`100% <100%> (ø)`
falcon/media/handlers.py	`100% <100%> (ø)`
falcon/util/reader.py	`100% <100%> (ø)`
falcon/media/multipart.py	`100% <100%> (ø)`
falcon/util/__init__.py	`100% <100%> (ø)`
falcon/media/base.py	`100% <100%> (ø)`

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 769de42...8793055. Read the comment docs.

CaselIT · 2020-01-25T19:49:43Z

I've a couple of questions:

should the kw argument be kw only? Using PEP 3102 def __init__(self, *, title=None, ...)?
some types have additional keyword args, that are placed in headers, like challenges in HTTPUnauthorized. I've not changed this. Should we change it?

kgriffs · 2020-01-25T23:11:58Z

should the kw argument be kw only? Using PEP 3102 def init(self, *, title=None, ...)?

Yes, I think that would be a Good Thing™. As an aside, we should consider doing this in other places as well so we don't have to worry about a simple reordering of kwargs down the road causing a breaking change. @vytas7 thoughts?

kgriffs · 2020-01-25T23:30:53Z

some types have additional keyword args, that are placed in headers, like challenges in HTTPUnauthorized. I've not changed this. Should we change it?

It would not hurt to normalize the ordering of extra params such that they always follow headers (although it would only be cosmetic if PEP 3102 is used). Regardless, we will need to make sure we have test coverage for things like that overriding any values also set in headers.

kgriffs · 2020-01-25T23:34:57Z

falcon/errors.py

@@ -377,11 +372,9 @@ class HTTPMethodNotAllowed(OptionalRepresentation, HTTPError):
            base articles related to this error (default ``None``).
    """

-    def __init__(self, allowed_methods, headers=None, **kwargs):
+    def __init__(self, allowed_methods, title=None, description=None, headers=None, **kwargs):


We'll need a breaking change news fragment since people may have been passing headers positionally.

I'll add it to the existing one regarding the NoRepresentation. Also the kw only change

btw towncrier does not properly render the misc changelog

CaselIT · 2020-01-25T23:51:05Z

we will need to make sure we have test coverage for things like that overriding any values also set in headers.

I think the current implementation overrides in all of them. I can check if that is the case and check if there are tests for it. Sadly the code coverage does not help in this case

kgriffs · 2020-01-26T00:59:46Z

docs/_newsfragments/777-refactor-errors.feature.rst

@@ -0,0 +1,2 @@
+Update to the error classes in ``falcon.errors`` to have ``title`` and


This needs just a slight rewording to be consistent with other fragments:

The error classes in ``falcon.errors`` were updated to have...

… values in headers

CaselIT · 2020-01-26T11:23:57Z

I'm changing to kwonly.

There seem to be a lot of changes required, both in code and in documentations.
Just letting you know

CaselIT · 2020-01-26T13:42:36Z

I've notices that for the headers the documentation says: headers (dict or list)

But the list case is not handled by all the errors that add something to the headers, like HTTPUnauthorized:

falcon/falcon/errors.py

Lines 165 to 170 in c817a42

    
           def __init__(self, title=None, description=None, challenges=None, headers=None, **kwargs): 
        
               if headers is None: 
        
                   headers = {} 
        
               if challenges: 
        
                   headers['WWW-Authenticate'] = ', '.join(challenges)

…rror

CaselIT · 2020-01-26T14:06:09Z

should be almost done other than #1655 (comment)

CaselIT · 2020-01-27T18:26:07Z

should the kw argument be kw only? Using PEP 3102 def init(self, *, title=None, ...)?

I know I'm the one who suggested it, and I've already implemented it, but thinking a bit more about it and also judging by the amount of refactoring that this has required in falcon, I'm not sure if it's worth the trouble. By it I mead that it will probably cause a lot of refactoring on the users who use falcon, without a clear improvement.
I think we can keep the changes in falcon, but I would revert the actual requirement for kw_only. Or at least move the * after the title and description.

What do you think @kgriffs @vytas7

vytas7 · 2020-01-27T18:44:51Z

I know I'm the one who suggested it, and I've already implemented it, but thinking a bit more about it and also judging by the amount of refactoring that this has required in falcon, I'm not sure if it's worth the trouble. By it I mead that it will probably cause a lot of refactoring on the users who use falcon, without a clear improvement.

I'd need to check signatures of the existing HTTPErrors & HTTPStatuses more carefully to provide an informed opinion.

A quick reaction off the top of my head would be that it indeed sounds like a bit too abrupt breaking change without any compatibility shims or deprecation warnings, even given that 3.0 will be a major release.

kgriffs · 2020-02-03T22:04:25Z

IMO we should at least add a warning section to the relevant docstrings. As for outputting a deprecation warning, I'm inclined to say yes as well, since this raising errors is typically not super performance sensitive, and the extra nudge to the app developer can't hurt. @vytas7 thoughts?

vytas7 · 2020-02-03T23:05:29Z

Agreed re the deprecation warning, not many are going to care or even notice about a line or two in the docs. It is ideal to emit them if possible, and agreed re the performance impact being lesser for exceptions.

CaselIT · 2020-02-03T23:08:24Z

ok, so something like the deprecate_arg in the snippet above

…em kwonly now

CaselIT · 2020-02-04T20:26:48Z

Ok I've replaced the kwonly with a warning and a note in the documents. (not sure if it should be a warning)

I think it's ready for review

kgriffs

Just a few minor things and this should be ready to merge!

falcon/util/misc.py

CaselIT · 2020-02-07T19:29:34Z

I've noticed that the changelog needs updating.

kgriffs · 2020-02-09T01:51:18Z

docs/_newsfragments/777.removal.rst

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


Seems like this deprecation could be its own news fragment?

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

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)

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

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?

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

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

kgriffs · 2020-02-09T01:52:17Z

I've noticed that the changelog needs updating.

Nice catch! I think we are all trying to get used to making news fragments part of the PRs.

CaselIT added 6 commits January 25, 2020 18:21

refactor(errors.py): unify init signature of the error classes

4d6873d

refactor(errors.py): remove NoRepresentation, HTTPRangeNotSatisfiable…

137cee8

… has optional representation

refactor(errors.py): use py3 style super

2ac519f

test(errors): add tests to changes in errors, update other tests

7bab132

chore(news): add news fragment for changelog

0ffb44b

docs: remove references to NoRepresentation in documentation

c98c7f3

kgriffs reviewed Jan 25, 2020

View reviewed changes

Merge branch 'master' into fix-777-errors

ea554a3

kgriffs reviewed Jan 26, 2020

View reviewed changes

vytas7 and others added 3 commits January 26, 2020 10:13

Merge branch 'master' into fix-777-errors

8a95c65

refactor(errors): make the arguments kwonly

569cabf

refactor(errors): ensure that the arguments of the error override the…

6695d36

… values in headers

CaselIT added 2 commits January 26, 2020 13:50

docs(errors): update error documentation adding kw arguments

a48e8c5

refactor(error): use kw args when creating errors

a55f940

CaselIT added 5 commits January 26, 2020 14:51

chore(cython): fix errors on cython

5e04469

refactor(errors): make the arguments kwonly in httperror

055cb04

docs(errors): update error documentation adding kw arguments to httpe…

992a2b3

…rror

chore(pep): fix pep errors

f3aefc4

docs(news): update new for changelog

2fef4e4

CaselIT requested a review from kgriffs January 26, 2020 14:06

CaselIT added 2 commits February 4, 2020 20:10

Merge branch 'master' into fix-777-errors

4109f27

refactor(errors): deprecate positional arguments instead of making th…

6969e0c

…em kwonly now

CaselIT force-pushed the fix-777-errors branch from b77821d to 6969e0c Compare February 4, 2020 20:25

CaselIT requested a review from vytas7 February 4, 2020 20:26

style(pep8): fix pep8 errors

89e137a

kgriffs requested changes Feb 5, 2020

View reviewed changes

falcon/util/misc.py Show resolved Hide resolved

falcon/util/misc.py Outdated Show resolved Hide resolved

falcon/util/misc.py Outdated Show resolved Hide resolved

falcon/util/misc.py Show resolved Hide resolved

falcon/util/misc.py Outdated Show resolved Hide resolved

CaselIT added 2 commits February 5, 2020 20:56

chore: add missing tests and address review

e875d15

docs: be polite

ab3645a

kgriffs previously approved these changes Feb 5, 2020

View reviewed changes

docs(changelog): update changelog

c13bd66

CaselIT dismissed kgriffs’s stale review via c13bd66 February 8, 2020 13:43

kgriffs reviewed Feb 9, 2020

View reviewed changes

docs(changelog): address review comments

9fef3b3

CaselIT requested a review from kgriffs February 11, 2020 19:06

Merge branch 'master' into fix-777-errors

769de42

kgriffs previously approved these changes Feb 12, 2020

View reviewed changes

kgriffs mentioned this pull request Feb 13, 2020

doc(changelog): reorganize towncrier #1621

Merged

11 tasks

Merge branch 'master' into fix-777-errors

8793055

kgriffs dismissed their stale review via 8793055 February 15, 2020 19:27

kgriffs merged commit 61d6424 into falconry:master Feb 15, 2020

CaselIT deleted the fix-777-errors branch February 15, 2020 19:54

CaselIT mentioned this pull request Feb 24, 2020

Error representation strangeness #452

Closed

vytas7 mentioned this pull request Mar 25, 2020

Docs. Example code updated #1702

Closed

13 tasks

CaselIT mentioned this pull request Apr 16, 2020

Improve error and status handling #1701

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Make error initializers more consistent #1655

Fix: Make error initializers more consistent #1655

CaselIT commented Jan 25, 2020 •

edited

codecov bot commented Jan 25, 2020 •

edited

CaselIT commented Jan 25, 2020 •

edited

kgriffs commented Jan 25, 2020

kgriffs commented Jan 25, 2020

kgriffs Jan 25, 2020

CaselIT Jan 25, 2020

CaselIT Jan 25, 2020

CaselIT commented Jan 25, 2020

kgriffs Jan 26, 2020

CaselIT commented Jan 26, 2020

CaselIT commented Jan 26, 2020 •

edited

CaselIT commented Jan 26, 2020

CaselIT commented Jan 27, 2020 •

edited

vytas7 commented Jan 27, 2020 •

edited

kgriffs commented Feb 3, 2020

vytas7 commented Feb 3, 2020

CaselIT commented Feb 3, 2020 •

edited

CaselIT commented Feb 4, 2020

kgriffs left a comment

CaselIT commented Feb 7, 2020

kgriffs Feb 9, 2020

CaselIT Feb 9, 2020 •

edited

kgriffs Feb 11, 2020

CaselIT Feb 11, 2020 •

edited

kgriffs Feb 11, 2020 •

edited

CaselIT Feb 11, 2020

CaselIT Feb 11, 2020

kgriffs commented Feb 9, 2020

		@@ -0,0 +1,2 @@
		Update to the error classes in ``falcon.errors`` to have ``title`` and

Fix: Make error initializers more consistent #1655

Fix: Make error initializers more consistent #1655

Conversation

CaselIT commented Jan 25, 2020 • edited

Summary of Changes

Related Issues

Pull Request Checklist

codecov bot commented Jan 25, 2020 • edited

Codecov Report

CaselIT commented Jan 25, 2020 • edited

kgriffs commented Jan 25, 2020

kgriffs commented Jan 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaselIT commented Jan 25, 2020

Choose a reason for hiding this comment

CaselIT commented Jan 26, 2020

CaselIT commented Jan 26, 2020 • edited

CaselIT commented Jan 26, 2020

CaselIT commented Jan 27, 2020 • edited

vytas7 commented Jan 27, 2020 • edited

kgriffs commented Feb 3, 2020

vytas7 commented Feb 3, 2020

CaselIT commented Feb 3, 2020 • edited

CaselIT commented Feb 4, 2020

kgriffs left a comment

Choose a reason for hiding this comment

CaselIT commented Feb 7, 2020

Choose a reason for hiding this comment

CaselIT Feb 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaselIT Feb 11, 2020 • edited

Choose a reason for hiding this comment

kgriffs Feb 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgriffs commented Feb 9, 2020

CaselIT commented Jan 25, 2020 •

edited

codecov bot commented Jan 25, 2020 •

edited

CaselIT commented Jan 25, 2020 •

edited

CaselIT commented Jan 26, 2020 •

edited

CaselIT commented Jan 27, 2020 •

edited

vytas7 commented Jan 27, 2020 •

edited

CaselIT commented Feb 3, 2020 •

edited

CaselIT Feb 9, 2020 •

edited

CaselIT Feb 11, 2020 •

edited

kgriffs Feb 11, 2020 •

edited