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 path_segments to PathAndSegments #276

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

20chan
Copy link

@20chan 20chan commented Nov 5, 2018

Add path_segments to PathAndSegments and add test_uri_parse_segments to tests.
And I just got noticed rustfmt auto formatted two code files.

@20chan 20chan mentioned this pull request Nov 5, 2018
Copy link
Collaborator

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started 👍

It looks like the PR contains unrelated formatting changes. Would it be possible to revert those?

src/uri/path.rs Outdated Show resolved Hide resolved
src/uri/path.rs Outdated Show resolved Hide resolved
src/uri/path.rs Outdated Show resolved Hide resolved
src/uri/path.rs Outdated
/// assert_eq!(path_and_query.path_segments().unwrap().collect::<Vec<_>>(), vec!["hello",
/// "world"])
/// ```
pub fn path_segments(&self) -> Option<str::Split<char>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably return a custom iterator: struct PathSegments<str::Split<...>>

Is Option significant in this case or can the iterator be returned directly? Uri::path_and_query already returns an Option.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, Option in this context is meanless.

Copy link
Author

Choose a reason for hiding this comment

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

And custom iterator you said, I need help.. I'm so new to rust, When I write any custom iterator, compiler raise lifetime error. T^T

src/uri/tests.rs Outdated Show resolved Hide resolved
src/uri/tests.rs Outdated Show resolved Hide resolved
@20chan
Copy link
Author

20chan commented Nov 6, 2018

All right, First, I reverted unrelated changes.

And also edited test case from docs
src/uri/path.rs Outdated
/// vec!["hello", "world"]
/// );
/// ```
pub fn path_segments(&self) -> str::Split<char> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you return a new type here? Some type that wraps str::Split<char> and implements Iterator:

struct PathSegments(str::Split<char>);

This is for forwards compatibility as it allows us to change the implementation details of the iterator.

I'm also on the fence about whether it should be path_segments or just segments. It is a method on the PathAndQuery type, so it could go either way...

Copy link
Author

Choose a reason for hiding this comment

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

Sorry to bother you, but I created one. Can you check?

Copy link
Collaborator

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks, sorry for the delay (I was on vacation), and thanks for sticking with it ❤️. Thoughts inline.

#[derive(Debug)]
pub struct PathSegments<'a>(str::Split<'a, char>);

impl<'a> std::ops::Deref for PathSegments<'a> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of implementing Deref, could you implement, could we implement Iterator<Item = &'a str> and DoubleEndedIterator? The impl would proxy to str::Slice<'a, char>'s implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also think that the the CI failure is due to referencing std instead of ::std.

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

Successfully merging this pull request may close these issues.

2 participants