Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Added new long route constraint and updated the int route constraint to int #1569

Merged
merged 1 commit into from
Sep 27, 2014

Conversation

phillip-haydon
Copy link
Member

Fix for #1565 because @mat-mcloughlin was too slow.

Note:

If you have two constraints on the same URL for int and long then first in wins since the scoring will be the same. But there's no way for us to truly know which route to pick.

If the user puts in 1000 but wants it to hit the long route then it may pick the int route.

But we can't prevent this, and the user is stupid if they're making routes like this anyway.

@phillip-haydon phillip-haydon added this to the 1.0Alpha milestone Jun 14, 2014
@@ -24,6 +24,23 @@ public void Should_resolve_int_constraint()
}

[Fact]
public void When_int_is_larger_than_max_int_should_has_no_match()
Copy link
Member

Choose a reason for hiding this comment

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

G/W/T to make @thecodejunkie happy 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

If you look at the tests, they don't constitute G/W/T, so its pointless.

Copy link
Member

Choose a reason for hiding this comment

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

Agree but I think he wants them regardless. In your tests I'd do //Given/When on the browser get and a //Then over the assert

Copy link
Member

Choose a reason for hiding this comment

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

I'd put // Givenin the ctor, // When over the the this.browser.Get and // Then over the assert

@horsdal
Copy link
Member

horsdal commented Sep 23, 2014

Apart from GWT nickpicking 👍

horsdal added a commit that referenced this pull request Sep 27, 2014
Added new long route constraint and updated the int route constraint to int
@horsdal horsdal merged commit 02ac868 into NancyFx:master Sep 27, 2014
khellang referenced this pull request Sep 28, 2014
add GWT comments to ConstraintNodeRouteResolverFixture
@khellang
Copy link
Member

Added the new constraint to the wiki 😄

@khellang
Copy link
Member

How is this a breaking change?

@phillip-haydon
Copy link
Member Author

https://github.com/NancyFx/Nancy/pull/1569/files#diff-beb661f87d0a8be138928b34c1530c1fR8

Because of this, the 'int' constraint changed from long to int and a new 'long' constraint was added.

If anyone was using int expecting larger than Int32 values it will break for them.

@khellang
Copy link
Member

👌

@drankard
Copy link

drankard commented Feb 2, 2015

What is the reason for this "fix" ?

A lot of refactoring, checking, casting and/or double routing is needed now.

I don't agree in the statement that "the user is stupid"
Most NUMBER routes are resource identifiers living in a database, that span from small int to long, hence called NUMBER, why not add a boxing datatype (as jchannon suggest in #1565) or let it be up to the developer to do the casting. if really needed ?

Am i missing something ?

@phillip-haydon
Copy link
Member Author

@drankard we could have added 'number' but then we have int and number, removing int we now have a major breaking change, and a higher barrier to entry when MVC supports int and long. jchannon also agreed with int/long approach further down. And while we could let the developer do the casting the request would be more expensive for them to execute the module definition rather than just deny the request up front.

The check for us is not expensive. Its potentially expensive for the module.

@grumpydev grumpydev modified the milestones: 1.0 Alpha, 1.0 Feb 4, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants