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

Fix HasUrlParameter serialization and deserialization #2576

Merged
merged 7 commits into from
Oct 3, 2017

Conversation

caalador
Copy link
Contributor

@caalador caalador commented Oct 2, 2017

Fixes urlParameter deserialization to work with all supported types.
Fixes serialization for getUrl.


This change is Reviewable

Fixes urlParameter deserialization to work with all supported types.
Fixes serialization for  getUrl.
@caalador
Copy link
Contributor Author

caalador commented Oct 2, 2017

This will fix demo-static-menu-router blogs Internal error.

@denis-anisimov
Copy link
Contributor

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


flow-server/src/main/java/com/vaadin/router/HasUrlParameter.java, line 77 at r2 (raw file):

if (!parameterType.isAssignableFrom(String.class)) {

Quoted 4 lines of code… > throw new UnsupportedOperationException( > "Wildcard parameter can only be for String type by default. Implement `deserializeUrlParameters` for class " > + this.getClass().getName()); > }

This code may be is going to be evolved. So please extract this into a separate method.


flow-server/src/main/java/com/vaadin/router/HasUrlParameter.java, line 89 at r2 (raw file):

String parameter = urlParameters.get(0);

Quoted 11 lines of code… > if (parameterType.isAssignableFrom(String.class)) { > return (T) parameter; > } else if (parameterType.isAssignableFrom(Integer.class)) { > return (T) Integer.valueOf(parameter); > } else if (parameterType.isAssignableFrom(Long.class)) { > return (T) Long.valueOf(parameter); > } else if (parameterType.isAssignableFrom(Boolean.class)) { > return (T) Boolean.valueOf(parameter); > } else { > throw new IllegalArgumentException("Bad type."); > }

This also looks like potentially evolving code.
I would say that even now it should work via number of strategies that are able to handle separate types on their own and shouldn't know about each other.
But for now please extract this as a separate method.
Should be enough for now.


flow-server/src/main/java/com/vaadin/router/HasUrlParameter.java, line 91 at r2 (raw file):

throw new IllegalArgumentException("Bad type.");

Not descriptive enough.


flow-server/src/main/java/com/vaadin/router/Router.java, line 199 at r2 (raw file):

Boolean

@link please.


flow-server/src/main/java/com/vaadin/router/Router.java, line 200 at r2 (raw file):

String

@link please.


flow-server/src/main/java/com/vaadin/router/Router.java, line 210 at r2 (raw file):

parameter

plural please


Comments from Reviewable

@caalador
Copy link
Contributor Author

caalador commented Oct 3, 2017

Review status: 1 of 4 files reviewed at latest revision, 7 unresolved discussions.


flow-server/src/main/java/com/vaadin/router/HasUrlParameter.java, line 77 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

if (!parameterType.isAssignableFrom(String.class)) {
throw new UnsupportedOperationException(
"Wildcard parameter can only be for String type by default. Implement deserializeUrlParameters for class "
+ this.getClass().getName());
}

This code may be is going to be evolved. So please extract this into a separate method.

Done.


flow-server/src/main/java/com/vaadin/router/HasUrlParameter.java, line 89 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

String parameter = urlParameters.get(0);
if (parameterType.isAssignableFrom(String.class)) {
return (T) parameter;
} else if (parameterType.isAssignableFrom(Integer.class)) {
return (T) Integer.valueOf(parameter);
} else if (parameterType.isAssignableFrom(Long.class)) {
return (T) Long.valueOf(parameter);
} else if (parameterType.isAssignableFrom(Boolean.class)) {
return (T) Boolean.valueOf(parameter);
} else {
throw new IllegalArgumentException("Bad type.");
}

This also looks like potentially evolving code.
I would say that even now it should work via number of strategies that are able to handle separate types on their own and shouldn't know about each other.
But for now please extract this as a separate method.
Should be enough for now.

Done.


flow-server/src/main/java/com/vaadin/router/HasUrlParameter.java, line 91 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

throw new IllegalArgumentException("Bad type.");

Not descriptive enough.

Done.


flow-server/src/main/java/com/vaadin/router/Router.java, line 199 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

Boolean

@link please.

Using @code as anyone coding this should know what a Boolean and a String is.


flow-server/src/main/java/com/vaadin/router/Router.java, line 200 at r2 (raw file):

Using @code as anyone coding this should know what a Boolean and a String is.
Using @code as anyone coding this should know what a Boolean and a String is.


flow-server/src/main/java/com/vaadin/router/Router.java, line 210 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

parameter

plural please

Done.


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@denis-anisimov denis-anisimov merged commit 34fd033 into master Oct 3, 2017
@denis-anisimov denis-anisimov deleted the issues/url_parameter_serialization branch October 3, 2017 07:47
@ZheSun88 ZheSun88 added this to the 1.0.0.alpha4 milestone Oct 4, 2017
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.

None yet

4 participants