Skip to content

HTTPCLIENT-1916: Add URIBuilder.removeParameter#283

Merged
ok2c merged 2 commits intoapache:masterfrom
peterdettman:pr_HTTPCLIENT-1916
Apr 13, 2021
Merged

HTTPCLIENT-1916: Add URIBuilder.removeParameter#283
ok2c merged 2 commits intoapache:masterfrom
peterdettman:pr_HTTPCLIENT-1916

Conversation

@peterdettman
Copy link
Contributor

https://issues.apache.org/jira/browse/HTTPCLIENT-1916 .

Note that I have marked the new method as "since 5.2".

@garydgregory
Copy link
Member

May you please test edge cases like null and ""?

@peterdettman
Copy link
Contributor Author

I'm happy to add such tests, but I'm a bit unclear what the expected behaviour ought to be. I'm inclined to require non-null, non-empty for param but actually other URIBuilder methods don't seem to exclude "" for a name (is that really permissible?). BasicNameValuePair guards only against null too).

@michael-o
Copy link
Member

Why is a remove method necessary in a builder? Coming from a pre-built URI?

@peterdettman
Copy link
Contributor Author

@michael-o Some rationale was given by the submitter; I am merely picking up a volunteers-wanted label.

@ok2c
Copy link
Member

ok2c commented Apr 13, 2021

I'm happy to add such tests, but I'm a bit unclear what the expected behaviour ought to be. I'm inclined to require non-null, non-empty for param but actually other URIBuilder methods don't seem to exclude "" for a name (is that really permissible?). BasicNameValuePair guards only against null too).

@peterdettman I think just null check should be enough for now. The issue of name/value pairs with an empty name should be dealt with consistently across the entire library.

@peterdettman
Copy link
Contributor Author

I've added a null guard for the argument, with accompanying test.

@ok2c ok2c merged commit 60d8940 into apache:master Apr 13, 2021
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.

4 participants