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.

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

Add support for negative values #1355

Merged
merged 2 commits into from Dec 1, 2018

Conversation

@Skuldur
Copy link
Contributor

@Skuldur Skuldur commented Sep 11, 2018

Added support for negative values in IntegerConverter and FloatConverter

  • Added unit tests for the negative values

If this method is not preferred, lets discuss the best way to add signed values.

Closes #729

@Skuldur
Copy link
Contributor Author

@Skuldur Skuldur commented Sep 13, 2018

There seems to be a problem with the Python 3.6 run of the CI, that check has been failing for all pull requests in the past month. Which is why the checks fail.

@ThiefMaster
Copy link
Member

@ThiefMaster ThiefMaster commented Sep 13, 2018

This is a breaking change as applications may not expect negative values there right now; you could either add a new converter or an argument (like it's done for the AnyConverter).

Also, general tip: Having issue numbers at the beginning of a commit message is more noise than useful (especially when not prefixed with # so they aren't linked on github).
Better to use a concise first commit message line and then a closes #729 somewhere in a later line of the commit message

@Skuldur Skuldur changed the title 729: Added support for negative values Add support for negative values Sep 13, 2018
@Skuldur
Copy link
Contributor Author

@Skuldur Skuldur commented Sep 13, 2018

I've changed the name, thanks for the tip.

In my opinion this is not a breaking change but rather a bug fix (or might be both). Int implies that the input should allow any integer, the fact that it hasn't until now should be considered a bug as it is not expected behaviour for an integer parser.

However, I realise that a lot of people might be using the fact it doesn't allow negative numbers to skip writing their own error handling for invalid values (in the case of paging) so I will make two new converters SignedIntegerConverter and SignedFloatConverter as that will probably cause the least amount of tension.

Do you prefer the new types to be called 'sint' and 'sfloat' or 'signed_int' and 'signed_float'? :)

@ThiefMaster
Copy link
Member

@ThiefMaster ThiefMaster commented Sep 13, 2018

Bugfixes can be breaking changes as well if the bug existed for a long time and people likely relied on them. For example, the most common use case for int URL segments are probably database IDs. Usually those start with 1 (or 0), but aren't negative. Common usecase for negative IDs is to indicate special object types (e.g. a system user in case of a user id) - but if these can suddenly be specified in a URL this could be bad.

If backwards compatibility was not thing I'd change int and add uint for positive integers. sint came to my mind but the name sounds pretty awful. Since most likely the common case is unsigned ints, What about making it an argument?

  • /foo/<int(signed):some_id>/
  • /foo/<int(-):some_id>/
  • /foo/<sint:some_id>/
  • /foo/<signed-int:some_id>/

If we go for sint, I'd also add uint as an alias for the old int so one can be explicit in his own code.

@Skuldur
Copy link
Contributor Author

@Skuldur Skuldur commented Sep 13, 2018

Adding an uint alias shouldn't be too hard. But sint might confuse people since it is commonly used for short integer. So I think we should not use sint for signed integer.

I think either
/foo/<int(signed):some_id>/
or
/foo/<signed-int:some_id>/

Should be the method we use. I have no strong preference for either one, but signed-int might be easier to add.

EDIT: Werkzeug does not allow - in their url rules so we can rule out signed-int so we're down to signed_int or adding (signed) as an argument.

@Skuldur
Copy link
Contributor Author

@Skuldur Skuldur commented Sep 14, 2018

I've updated the method of support to

/foo/<int(signed=True):some_id>/

tests/test_routing.py Outdated Show resolved Hide resolved
@Skuldur
Copy link
Contributor Author

@Skuldur Skuldur commented Sep 19, 2018

Any other comments?

@davidism davidism added this to the 0.15 milestone Dec 1, 2018
@davidism davidism force-pushed the 729-add-negative-number-support branch from 1e3b4ce to 8c7efbe Dec 1, 2018
@davidism
Copy link
Member

@davidism davidism commented Dec 1, 2018

Rebased, added changelog, and refactored. signed is now a parameter of NumberConverter, and allows overriding the signed_regex attribute.

@davidism davidism merged commit 845a40a into pallets:master Dec 1, 2018
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants