option: Disallow username in URL #2340

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@zagor
Member

zagor commented Feb 25, 2018

This solves TODO 1.1.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 26, 2018

Member

This needs a test, it can be fairly straightforward I think, just test the return code in the fail case. See test2044 for a very simple example. (edit: on second thought you'll probably need to start the HTTP server in the test so that you have a valid URL to pass)

CI says add to symbols-in-versions and document in cmdline-opts, and tool_help.c. I would rename the tool switch from --disallow-username to --disallow-username-in-url. It's longer but for someone reading a script it's easier to understand what it does.

I suggest not using TODO: as the area because it reads like you're adding something to the todo. sample alternatives
lib: Add option to disallow usernames in URLs
url: Add option CURLOPT_DISALLOW_USERNAME_IN_URL
etc

Member

jay commented Feb 26, 2018

This needs a test, it can be fairly straightforward I think, just test the return code in the fail case. See test2044 for a very simple example. (edit: on second thought you'll probably need to start the HTTP server in the test so that you have a valid URL to pass)

CI says add to symbols-in-versions and document in cmdline-opts, and tool_help.c. I would rename the tool switch from --disallow-username to --disallow-username-in-url. It's longer but for someone reading a script it's easier to understand what it does.

I suggest not using TODO: as the area because it reads like you're adding something to the todo. sample alternatives
lib: Add option to disallow usernames in URLs
url: Add option CURLOPT_DISALLOW_USERNAME_IN_URL
etc

@zagor zagor changed the title from TODO 1.1: Disallow usernames in URLs to option: Disallow username in URL Mar 2, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 3, 2018

Member

It seems the new test case 2074 fails sometimes? Is it because of different build options?

Member

bagder commented Mar 3, 2018

It seems the new test case 2074 fails sometimes? Is it because of different build options?

@bagder bagder added the URL label May 23, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 23, 2018

Member

@zagor Any chance you're up for rebasing this and fixing the conflict?

Member

bagder commented May 23, 2018

@zagor Any chance you're up for rebasing this and fixing the conflict?

@zagor

This comment has been minimized.

Show comment
Hide comment
@zagor

zagor May 23, 2018

Member

Absolutely!

Member

zagor commented May 23, 2018

Absolutely!

@bagder bagder closed this in 946ce5b May 31, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 31, 2018

Member

Thanks! (slightly edited before I pushed)

Member

bagder commented May 31, 2018

Thanks! (slightly edited before I pushed)

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