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

Add option to use error message provider for StringTo converters #4202

Merged
merged 3 commits into from
May 31, 2018

Conversation

pekam
Copy link
Contributor

@pekam pekam commented May 29, 2018

Ported change from vaadin/framework#10711

Only difference is in the added unit test: Flow components don't have setLocale() method like in FW8, and as there is no UI present, I had to change the default locale.


This change is Reviewable

@denis-anisimov
Copy link
Contributor

@pekam
Copy link
Contributor Author

pekam commented May 30, 2018

Review status: all files reviewed at latest revision, 7 unresolved discussions.


flow-data/src/main/java/com/vaadin/flow/data/converter/StringToBigDecimalConverter.java, line 83 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
given empty string

No empty string. It's a BigDecimal.

Nice catch.

But I'm not sure about the phrasing. I mean, it's not empty BigDecimal value. It's more like the BigDecimal value that is shown if the string is empty.

Would you agree if I change them all to:
Creates a new converter instance with the given presentation value for empty string and error message


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


flow-data/src/main/java/com/vaadin/flow/data/converter/StringToBigDecimalConverter.java, line 83 at r1 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

Nice catch.

But I'm not sure about the phrasing. I mean, it's not empty BigDecimal value. It's more like the BigDecimal value that is shown if the string is empty.

Would you agree if I change them all to:
Creates a new converter instance with the given presentation value for empty string and error message

That's OK for me.


Comments from Reviewable

@pekam
Copy link
Contributor Author

pekam commented May 31, 2018

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


flow-data/src/main/java/com/vaadin/flow/data/converter/AbstractStringToNumberConverter.java, line 49 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
the given empty string

copy/paste from the original FW8 PR but there is no a string in the CTOR.
Needs to be corrected.

Done.


flow-data/src/main/java/com/vaadin/flow/data/converter/StringToBigDecimalConverter.java, line 83 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

That's OK for me.

Done.


flow-data/src/main/java/com/vaadin/flow/data/converter/StringToBigIntegerConverter.java, line 84 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
given empty string

Not a string.

Done.


flow-data/src/main/java/com/vaadin/flow/data/converter/StringToDoubleConverter.java, line 80 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
given empty string

It's a Double value.

Done.


flow-data/src/main/java/com/vaadin/flow/data/converter/StringToFloatConverter.java, line 78 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
given empty string 

It's a Float value.

Done.


flow-data/src/main/java/com/vaadin/flow/data/converter/StringToIntegerConverter.java, line 77 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
given empty string value

It's an int.

Done.


flow-data/src/main/java/com/vaadin/flow/data/converter/StringToLongConverter.java, line 77 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
given empty string value

It's a long.

Done.


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

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

3 participants