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 ZoneId converter #24

Merged
merged 2 commits into from Jul 13, 2022
Merged

Add ZoneId converter #24

merged 2 commits into from Jul 13, 2022

Conversation

pavanpvpk
Copy link

No description provided.

@gkopff
Copy link
Owner

gkopff commented Jan 11, 2019

Hi @pavanpvpk - sorry to take so long to get back to you, I've been travelling.

I have a couple of points of feedback:

  • could we please limit the PR to the code change, and leave out the various IntelliJ file changes. If those are useful to propagate forward, I think they should go in a separate PR
  • could you please match the style (brace location, indentation, whitespace etc) with the existing source code
  • I don't think its warranted to add a dependency on commons-lang3 for a single method to check if a string is blank. If we require this logic (not null, not empty, not whitespace), we can add that longhand ourselves. My opinion however is that isBlank is too "wide" - I can accept that a null value convert to null, and that an empty string convert to null, but I don't think whitespace should convert successfully to anything.

@pavanpvpk
Copy link
Author

@gkopff extremely sorry for the delay. I have made the changes and added few new test cases as well. Let me know your feedback.

@ahmedkhalf
Copy link

@gkopff Yo, any idea idea when this gonna be pushed? I have been impatiently waiting for three years.

@gkopff gkopff changed the base branch from master to issue-24 July 13, 2022 03:32
@gkopff gkopff merged commit 2fb0fbb into gkopff:issue-24 Jul 13, 2022
@gkopff gkopff mentioned this pull request Jul 13, 2022
@gkopff
Copy link
Owner

gkopff commented Jul 13, 2022

@ahmedkhalf the formatting fixes never got done, so that's why it was sitting there. I've fixed the formatting myself today - but it will be a few days to work out pushing to Maven Central. Things have changed since I last did it.

@gkopff
Copy link
Owner

gkopff commented Jul 16, 2022

@ahmedkhalf This has been released, but due to a backlog in the Maven Central indexing, it's not yet reported by the search. It should be resolvable by Maven itself though - I've bene in contact with Sonatype about it: https://issues.sonatype.org/browse/OSSRH-82669

@gkopff
Copy link
Owner

gkopff commented Jul 17, 2022

@ahmedkhalf
Copy link

ahmedkhalf commented Jul 17, 2022

Thank you!

But now even though the tests are passing I am getting a warning:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.gson.internal.reflect.ReflectionHelper (file:/home/ahmedk/.m2/repository/com/google/code/gson/gson/2.9.0/gson-2.9.0.jar) to field java.time.ZoneRegion.id
WARNING: Please consider reporting this to the maintainers of com.google.gson.internal.reflect.ReflectionHelper
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

After some investigation, I decided to use reflection to get the private class in java.time that was causing issues:

builder.registerTypeAdapter(getClassByName("java.time.ZoneRegion"), new ZoneIdConverter());

not sure if this is the best way to go about this...

should I file an issue?

@gkopff
Copy link
Owner

gkopff commented Jul 17, 2022

As the warning says, that's a Gson issue. I don't think Gson will ever address it. Moshi appears to be the successor to Gson, as per: google/gson#1821

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