Skip to content

feat: add url_components for start/end #250

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Mar 3, 2023
Merged

feat: add url_components for start/end #250

merged 14 commits into from
Mar 3, 2023

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Mar 1, 2023

No description provided.

@anonrig anonrig requested a review from lemire March 1, 2023 13:37
@anonrig anonrig force-pushed the feat-add-url-components branch 2 times, most recently from 1f3f36a to 037f54b Compare March 1, 2023 14:31
@lemire
Copy link
Member

lemire commented Mar 1, 2023

Let me try to do a PR on top to express my ideas.

@lemire lemire mentioned this pull request Mar 1, 2023
Merged
@lemire
Copy link
Member

lemire commented Mar 1, 2023

Please see #251

@lemire
Copy link
Member

lemire commented Mar 1, 2023

With such a component, it could be fruitful to restructure the main ada/url. It could be made of a single string (which might possibly be identical to the input) along with this url_component. It could make the main ada/url structure smaller and faster. We could then boost our performance (e.g., by 25% or so).

@lemire lemire mentioned this pull request Mar 1, 2023
Merged
lemire
lemire approved these changes Mar 1, 2023
View reviewed changes
Copy link
Member

@lemire lemire left a comment

Choose a reason for hiding this comment

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

The validation might be insufficient (???), but I like this PR a lot. If we can make sure that it is correct, it is a great step forward.

Note that it is not documented in the README (and I think that's good at this time to be quiet about it).

@anonrig
Copy link
Member Author

anonrig commented Mar 1, 2023

We also do not have all the tests for this. Let's finish them before merging this.

@lemire
Copy link
Member

lemire commented Mar 1, 2023

It would be good to have a plan to see where this is going. I started a discussion issue: #253

@anonrig anonrig marked this pull request as ready for review March 2, 2023 19:43
@anonrig
Copy link
Member Author

anonrig commented Mar 2, 2023

We are using a combination of 0 or omitted for the default values. We need to make sure the default values are properly tested too...

lemire
lemire reviewed Mar 3, 2023
View reviewed changes
@anonrig anonrig force-pushed the feat-add-url-components branch from e53ea26 to 9dacf36 Compare March 3, 2023 15:19
@anonrig anonrig merged commit 4d2a742 into main Mar 3, 2023
@anonrig anonrig deleted the feat-add-url-components branch March 3, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lemire lemire lemire approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anonrig @lemire