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

Adding the QueryItem struct in path #268

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adding the QueryItem struct in path #268

wants to merge 1 commit into from

Conversation

yageek
Copy link

@yageek yageek commented Oct 29, 2018

As requested in #206, building an URI from a builder should have a smaller granularity.
A struct QueryItem has been added. It holds the elements of a query component.
A method new inside PathAndQuery has been added. It that requires a path and a list of QueryItem and build a new PathAndQuery from them.

The URI builder could hold both a String and a Vec<QueryItem> fields instead of one parts field and could provide new methods to configure the path and the query separately.

As requested in #206, building an URI from a builder should have a
smaller granularity.

A struct `QueryItem` has been added. It holds the
elements of a query component.

A method `new` inside `PathAndQuery` has been added.
It  that requires a `path` and a list of `QueryItem` and
build a new `PathAndQuery` from them.

The URI builder could hold both a `String` and a `Vec<QueryItem>` fields
instead of one `parts` field and could provide new methods
to configure the path and the query separately.
@seanmonstar
Copy link
Member

Thanks @yageek for thinking about this! I think we should discuss a bit more what the API should look like.

  • Assuming that this should be an addition to the Builder, we should consider what new method(s) should be added.
  • I'd suggest that this shouldn't (yet) add PathAndQuery::new, as that's a pretty coveted constructor name. We should start with just the Builder.
  • It's preferred that any public struct have its fields be private, so that internal representations can be changed without worry of breaking changes.

(If you'd rather discuss these on the tracking issue #206, that's fine too!)

@yageek
Copy link
Author

yageek commented Oct 30, 2018

I think you're right, I may attack the problem in the wrong order. I'll put some comments on #206

@yageek yageek mentioned this pull request Nov 6, 2018
8 tasks
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.

None yet

2 participants