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
base: master
from

Conversation

Projects
None yet
3 participants
@bagder
Member

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 changed the title from API for parsing URLs and extrating/setting individual URL parts to API for parsing URLs and extracting/setting individual URL parts Aug 6, 2018

@xquery

This comment has been minimized.

Contributor

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)

This comment has been minimized.

@vszakats

vszakats Aug 7, 2018

Member

Minor nit: An unnecessary empty line slipped in here.

This comment has been minimized.

@bagder

bagder Aug 7, 2018

Member

oops

@vszakats

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

bagder commented Aug 7, 2018

More minor fixes, squashed and rebased.

.B #include <curl/curl.h>
.nf
CURLUcode curl_url(char *url,

This comment has been minimized.

@vszakats

vszakats Aug 7, 2018

Member

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

@bagder

This comment has been minimized.

Member

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

URL-API
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes #2842
@bagder

This comment has been minimized.

Member

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

URL-API
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes #2842
@bagder

This comment has been minimized.

Member

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

URL-API
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 from API for parsing URLs and extracting/setting individual URL parts to URL API Aug 13, 2018

bagder added a commit that referenced this pull request Aug 13, 2018

URL-API
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

URL-API
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

URL-API
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

URL-API
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 Sep 8, 2018

falconindy added a commit to falconindy/curl that referenced this pull request Sep 10, 2018

URL-API
See header file and man pages for API. All documented API details work
and are tested in the 1560 test case.

Closes curl#2842
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment