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

URL API #2842

Closed
wants to merge 1 commit into from
Closed

URL API #2842

wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Aug 6, 2018

The URL API. Man pages with all the details and a fair amount of tests are included. I encourage all interested parties to try it out and report your experiences, both good and bad!


I'm saving these items until after the initial merge and done as separate PRs:

  • make libcurl use this parser internally too to avoid different treatment
  • add CURLOPT_CURLU to allow passing in a URL handle to work with
  • CURLU_CANONICAL - if there's a desire.

@bagder bagder added feature-window A merge of this requires an open feature window on-hold URL labels Aug 6, 2018
@bagder bagder changed the title API for parsing URLs and extrating/setting individual URL parts API for parsing URLs and extracting/setting individual URL parts Aug 6, 2018
@xquery
Copy link
Member

xquery commented Aug 7, 2018

looks good to me, there is some good inspiration over at https://github.com/uriparser/uriparser/blob/master/test/test.cpp
for other test variants.

@@ -95,3 +95,4 @@ unit1608_CPPFLAGS = $(AM_CPPFLAGS)

unit1609_SOURCES = unit1609.c $(UNITFILES)
unit1609_CPPFLAGS = $(AM_CPPFLAGS)

Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: An unnecessary empty line slipped in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops

@vszakats
Copy link
Member

vszakats commented Aug 7, 2018

Is there any reason why const isn't used in this prototype, for URL?:

CURLUcode curl_url(char *URL, CURLURL **urlhandle, unsigned int flags)

This function makes a call to parseurl(), where a const could added as well, and where internally the buffer isn't written nor passed to functions that write into it. ...it's possible I missed something reading the code.

UPDATE: Forgot to say: Thanks for this API, it is very useful!

@bagder
Copy link
Member Author

bagder commented Aug 7, 2018

No reason at all really. It's just an old weakness of mine to skip out on the 'const's... I'll fix. Thanks!

@bagder
Copy link
Member Author

bagder commented Aug 7, 2018

More minor fixes, squashed and rebased.

.B #include <curl/curl.h>

.nf
CURLUcode curl_url(char *url,
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the constifying, it's looking good! One minor: curl_url() prototype above needs to be synced with this change.

@bagder
Copy link
Member Author

bagder commented Aug 8, 2018

I think coveralls got some hiccups there when it claims a -3.4% less test coverage, as this patch provides quite a bit of testing already. I'm ignoring that for now, as I hope to add more tests anyway as I go along and fix the remaining parts.

bagder added a commit that referenced this pull request Aug 8, 2018
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes #2842
@bagder
Copy link
Member Author

bagder commented Aug 8, 2018

I'm not entirely happy with how curl_url() works just now, as just now we have two ways we can set a URL to the handle. I'm switching from:

Current

  CURLURL *h;
  rc = curl_url("http://example.com", &h, 0); /* create handle *

I suspect that way of storing the handle pointer is going to cause users grief. A bit too unusual.

New

  CURLURL *h = curl_url(); /* create handle */
  curl_url_set(h, CURLUPART_URL, "http://example.com", 0);

It is more in style with how our other APIs work and makes sure there's one single way to set URL in the handle,

Syntax-check a URL

A downside with this approach is that there's no longer any "shortcut" for just syntax-checking a URL without creating a handle, but then again I'm not convinced that's a very important feature.

bagder added a commit that referenced this pull request Aug 8, 2018
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes #2842
@bagder
Copy link
Member Author

bagder commented Aug 9, 2018

I've synced the wiki page to match how the current patch implements the API.

bagder added a commit that referenced this pull request Aug 9, 2018
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes #2842
@bagder bagder changed the title API for parsing URLs and extracting/setting individual URL parts URL API Aug 13, 2018
bagder added a commit that referenced this pull request Aug 13, 2018
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes #2842
bagder added a commit that referenced this pull request Aug 18, 2018
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes #2842
bagder added a commit that referenced this pull request Aug 23, 2018
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes #2842
@bagder bagder removed the on-hold label Sep 3, 2018
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes #2842
@bagder bagder closed this in fb30ac5 Sep 8, 2018
@bagder bagder deleted the URL-API branch September 8, 2018 13:36
falconindy pushed a commit to falconindy/curl that referenced this pull request Sep 10, 2018
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes curl#2842
@lock lock bot locked as resolved and limited conversation to collaborators Dec 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-window A merge of this requires an open feature window URL
Development

Successfully merging this pull request may close these issues.

None yet

3 participants