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

remove parameters from PlaceRequest #489

Closed
danielkaneider opened this issue May 14, 2014 · 7 comments
Closed

remove parameters from PlaceRequest #489

danielkaneider opened this issue May 14, 2014 · 7 comments
Assignees

Comments

@danielkaneider
Copy link
Contributor

Hi,

a common use case for URL navigation is to take the current PlaceRequest and to update some arguments. It might be the case that you want to remove one of the arguments.

If you start with an URL

..?a=1&b=2

you could want to following

..?a=1

Therefore PlaceRequest, or better PlaceRequest.Builder should allow to remove a specific key. At least it should not prevent this if you call

builder.with("b", null)

At the moment the null-check in the Builder prevents a NPE in ParameterTokenFormatter. But anyway, either allow to pass a null value in the with-method, or add a remove method to the builder.

Thanks,
Daniel

@christiangoudreau
Copy link
Member

I'm not completely convinced by this. The builder in itself starts from a clean PlaceRequest where you can pass another PlaceRequest to initialize it's parameter. In the event where you only need partial information to be added, why don't you added them one by one instead of trying to "remove" them?

@bmoritz
Copy link
Contributor

bmoritz commented May 15, 2014

I second Christian's opinion; maybe you could provide us with a more complete code example, so we can see the whole use case for this feature?

@jDramaix
Copy link
Contributor

I agree with Daniel, we should have a way to exclude some parameters when we initialize a builder with another place request. You could be in a case where you know that you have to remove one parameter but you don't know the others parameters present in the url. So you have to loop yourself over all parameters. GWTP could do that for us :-)

We could add a constructor like this:

public Builder(PlaceRequest request, String... excludedParameters) {
}

or even add a without(String parameterName)method. That makes sense to me as the builder is object used to build the PlaceRequest, so you can decide to add or remove parameters before to "freeze" the state of the PlaceRequest (when we call the build() method).

@danielkaneider
Copy link
Contributor Author

GWTP allows you to nest several presenters using slots. In such a case it might be, that the outer presenter (the one which defines the slot) wants to modify the URL without knowing other parameters added by child presenters. An example would be a presenter with a LEFT and RIGHT slot, with two independent presenters, where both modify parts of the URL. Adding parameters one by one means that all presenters have to know each others. This introduces a high level of coupling which goes against the slot concept, IMHO.

I think that making the with method null aware (see my patch), and adding a remove method to the builder would be the cleanest solutions. Patching the constructor of the builder means that you aren't able to really modify the builder after its creation.

@jDramaix
Copy link
Contributor

it's why I propose to do both. Patching the constructor and adding a method without(String parameterName). I'm against patching the with() method to accept null, that makes any sense in a fluent api.
I prefer to see:

new Builder(myPlaceRequest).without("foo").with("bar", "doe").build();

than

new Builder(myPlaceRequest).with("foo", null).with("bar", "doe").build();

@christiangoudreau
Copy link
Member

I agree with @jDramaix proposition

meriouma added a commit that referenced this issue Jun 3, 2014
meriouma added a commit that referenced this issue Jun 4, 2014
#489 and #492 : Adding .without() to PlaceRequest.Builder and fixed shared parameters map
@christiangoudreau
Copy link
Member

Closing, this has been fixed. Reopen if not.

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

No branches or pull requests

5 participants