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

release from current master? #40

Closed
jbr opened this issue Jun 18, 2020 · 3 comments
Closed

release from current master? #40

jbr opened this issue Jun 18, 2020 · 3 comments

Comments

@jbr
Copy link
Member

jbr commented Jun 18, 2020

Hi! I was investigating a tide routing issue (http-rs/tide#593) and discovered that it's resolved on master but not in 0.1.13. It must have been a subtle bug, because it doesn't seem like any of the changes between master and 0.1.13 directly address this.

This is a route-recognizer test case that fails in 0.1.13 but passes in master:

#[test]
fn ambiguous_router_wildcard_vs_star() {
    let mut router = Router::new();
    router.add("/:one/:two", "one/two".to_string());
    router.add("/posts/*", "posts/*".to_string());
    let id = router.recognize("/posts/10").unwrap();
    assert_eq!(*id.handler, "posts/*".to_string());
}
/*
0.1.13: 
---- ambiguous_router_wildcard_vs_star stdout ----
thread 'ambiguous_router_wildcard_vs_star' panicked at 'assertion failed: `(left == right)`
  left: `"one/two"`,
 right: `"posts/*"`'

Master: passes
*/

As a side note, it also seems that there's a breaking change regarding anonymous wildcards. This test case passes in 0.1.13, but fails on master:

#[test]
fn anonymous_wildcard() {
    let mut router = Router::new();
    router.add("/a/:/:c", "".to_string());
    let route = router.recognize("/a/10/20").unwrap();
    assert_eq!(route.params, two_params("", "10", "c", "20"));
}

/*
0.1.13: Passes

Master:
---- tests::anonymous_wildcard stdout ----
thread 'tests::anonymous_wildcard' panicked at 'assertion failed: `(left == right)`
  left: `Params { map: {"c": "20"} }`,
 right: `Params { map: {"": "10", "c": "20"} }`'
*/

I don't think this regression is especially important for tide, but it would be very nice to have a release with the first fix.

Thanks!

@Nemo157
Copy link
Contributor

Nemo157 commented Jun 18, 2020

Those are both breaking changes, existing users may be relying on the current route ordering. They were part of the planned 0.2 release (#28), but given I'm no longer part of the org I'm not sure if there's anyone pushing to get that complete.

@jbr
Copy link
Member Author

jbr commented Jun 18, 2020

Thanks, that makes sense. Are all of those changes required for a 2.0 release, if it's been almost a year without any progress? I'm happy to help move that forward, but it would be great to have a release of some sort in the mean time with the changes that already exist in master so tide users no longer experience unexpected behavior

@jbr jbr changed the title Patch release from master? release from current master? Jun 18, 2020
@yoshuawuyts
Copy link
Member

http-rs/tide#607 won't work for us, so probably releasing a 0.2 is okay -- it's been a while since we put out a release anyway. If we expect more breaking changes to come through, we can publish a 0.3 with them.

@jbr jbr closed this as completed Jun 19, 2020
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

No branches or pull requests

3 participants