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

fix : TODO the AllowMissing shim doesn't appear to offer much over Permissive. #1259

Closed
wants to merge 25 commits into from

Conversation

chidea
Copy link

@chidea chidea commented May 23, 2020

AllowMissing might be different from Permissive on special match cases "" and "/" but be the same on others.
This PR is making it return AllowMissing(None) on these special cases and AllowMissing(Some(_)) on others.
Some test cases are given to assert that both AllowMissing and Permissive act the same or the different correctly on some match cases.
A probable misinformation in the documentation of AllowMissing is also corrected.

@chidea chidea requested a review from hgzimmerman as a code owner May 23, 2020 15:21
/// Allows a section to match, providing a None value,
/// if its contents are entirely missing, or starts with a '/'.
/// if its contents are entirely missing, or ends with a '/' without any.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not exactly sure what without any refers to.

Copy link
Author

@chidea chidea May 23, 2020

Choose a reason for hiding this comment

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

It's the contents without any.
Sorry, I'm not a native.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's no problem!
I just want to make it clear for people who stumble across this in the future :)
Do you mean that there's nothing after the slash (i.e. / but not /<something here>

Copy link
Author

Choose a reason for hiding this comment

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

Yes it is.

Copy link
Contributor

@teymour-aldridge teymour-aldridge May 23, 2020

Choose a reason for hiding this comment

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

Suggested change
/// if its contents are entirely missing, or ends with a '/' without any.
/// if it doesn't have any contents, or is a pure path (i.e. doesn't have any query strings such as `?b=1` or fragments such as `#fragment`).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, it's more readable now.
Probably matches would be better than characters because there's a case like /# which also ends as AllowMissing(None).

Copy link
Author

Choose a reason for hiding this comment

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

Or matched characters would be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps mentioning which characters don't count?

Copy link
Author

Choose a reason for hiding this comment

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

Well, cases like #a=1 and ?b=1 would also be ignore. There are just too many cases that could be ignored.
Besides, this chracter matching is what matcher part of code does.
Too much information on another file would make people confused.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense.

@jstarry jstarry added the A-yew-router Area: The yew-router crate label May 28, 2020
@jstarry
Copy link
Member

jstarry commented Aug 15, 2020

@chidea sorry for the major delay in the review. I'm wondering if you'd be willing to move your changes to Classes into a separate PR so that this one can focus on fixing the router TODO.

EDIT: actually the best thing is to rebase now that #1448 has been merged

@jstarry
Copy link
Member

jstarry commented Sep 20, 2020

@chidea I personally feel that there shouldn't be a distinction between AllowMissing and Permissive behaviour because it's too confusing. Permissive to me allows having an optional wild card matcher. Here's an example using a regex style matcher that shows what I think the behaviour should be:

    #[to = "/{(.*)?}"]
    NotFound(Option<String>), // could be used to redirect the page but capture the original bad url

And AllowMissing to me should allow skipping certain arguments if you want:

    #[to = "/accounts/{id}/posts/{post_id?}", ignoreTrailingSlash]
    Accounts(String, Option<String>),

I think it requires a big rethinking of the Switch derive macro. We can add syntax like ? to indicate optional arguments and other options like ignoreTrailingSlash or exact to change other matching behaviour

@jstarry
Copy link
Member

jstarry commented Sep 20, 2020

@siku2 do you have any thoughts here? I'm thinking we could start a new project board for router improvements, it needs some love in my opinion

@jstarry jstarry closed this Sep 20, 2020
@jstarry
Copy link
Member

jstarry commented Sep 20, 2020

This work could be captured by #1107

@siku2
Copy link
Member

siku2 commented Sep 20, 2020

@jstarry I've spotted a lot of things in yew-router that could be improved. Adding a new project board makes sense!

@jstarry
Copy link
Member

jstarry commented Sep 20, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-router Area: The yew-router crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants