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

Conversion example inconsistent with conversion description #633

Closed
Azquelt opened this issue Oct 19, 2020 · 8 comments · Fixed by #643
Closed

Conversion example inconsistent with conversion description #633

Azquelt opened this issue Oct 19, 2020 · 8 comments · Fixed by #643
Labels
bug 🪲 An error in the implementation code or documentation

Comments

@Azquelt
Copy link
Member

Azquelt commented Oct 19, 2020

Describe the bug

The Config Value Conversion Rules section of the spec includes this:

Value Convert To Method Result
"\," String getValue "\,"
"\," String[] getValue throws NoSuchElementException
"\," String getOptionalValue Optional.of("\,")
"\," String[] getOptionalValue Optional.empty()
"\," String getOptionalValues Optional.empty()

I think this is wrong. Since \ is used as an escape character, the list style conversions should treat the , as a literal character, rather than a separator, so the result for getValue with String[] would be {","}.

I'm unsure whether this table should also have code tags around almost every entry. (My only doubt here is that \ is also the string escape in Java, so it could be confusing as to whether we're using Java's string syntax or not if it's formatted as code)

@Azquelt Azquelt added the bug 🪲 An error in the implementation code or documentation label Oct 19, 2020
@Azquelt
Copy link
Member Author

Azquelt commented Oct 19, 2020

I note that this case was listed as 3(b) in #446, and I'm not looking to reopen that discussion, but it does seem like the example in the spec contradicts the description of the rules here and here.

When I read the example, I assumed it was just a copy-paste error, but I'm happy for the other parts of the spec to change if the example is correct.

@Azquelt
Copy link
Member Author

Azquelt commented Oct 19, 2020

Smallrye Config has a specific check to not add a value of , to the array as the last step of converting the value: https://github.com/smallrye/smallrye-config/blob/7258a167a3f3ab26839749a37485bdf0edf749fe/implementation/src/main/java/io/smallrye/config/Converters.java#L718

I.e. "\," is converted to a single element "," which is then ignored because it's a single comma.

@Emily-Jiang
Copy link
Member

@Azquelt in this table you quoted, can you point out which row is wrong. Do you just say \, 4b is inconsistent or any row in the table you quoted is wrong?

@Azquelt
Copy link
Member Author

Azquelt commented Oct 26, 2020

Sorry, I just realised that github's formatting had removed my backslashes, making it make no sense. I've now corrected it.

I think that rows 2, 4 and 5 of the part of the table that I quoted are wrong.

They should be

Value Convert To Method Result
"\," String[] getValue {","}
"\," String[] getOptionalValue Optional.of({","})
"\," String getOptionalValues Optional.of(List.of(","))

@Emily-Jiang
Copy link
Member

The suggestion from @Azquelt sounds reasonable to me, since \ is an escape character for array of strings. @radcortez @jbee thoughts?

@jbee
Copy link
Contributor

jbee commented Oct 29, 2020

Sounds reasonable to me.

@radcortez
Copy link
Contributor

Ok.

@Emily-Jiang
Copy link
Member

Thanks @radcortez @jbee! Let me propose a PR to get it fixed.

radcortez added a commit to radcortez/microprofile-config that referenced this issue Nov 3, 2020
Signed-off-by: Roberto Cortez <radcortez@yahoo.com>
Emily-Jiang pushed a commit that referenced this issue Nov 3, 2020
Signed-off-by: Roberto Cortez <radcortez@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 An error in the implementation code or documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants