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

java.time needs to be better supported in Grails 7 #345

Open
codeconsole opened this issue Nov 2, 2024 · 10 comments · Fixed by #346
Open

java.time needs to be better supported in Grails 7 #345

codeconsole opened this issue Nov 2, 2024 · 10 comments · Fixed by #346
Assignees

Comments

@codeconsole
Copy link
Contributor

codeconsole commented Nov 2, 2024

We can start with a compiler configuration pending the outcome of this issue

Need a smooth pathway from legacy java.util.Date that does not requiring adding imports all over the place.

https://groovy.apache.org/blog/groovy-dates-and-times-cheat

Can be turned off using

grails {
    importJavaTime = false
}

Groovy currently star imports
"java.lang.", "java.util.", "java.io.", "java.net.", "groovy.lang.", "groovy.util."

Results in the following automatically imported:

Clock
DateTimeException
DayOfWeek
Duration
Instant
InstantSource
LocalDate
LocalDateTime
LocalTime
Month
MonthDay
OffsetDateTime
OffsetTime
Period
Year
YearMonth
ZonedDateTime
ZoneId
ZoneOffset
@matrei
Copy link
Contributor

matrei commented Nov 4, 2024

I think this is a sensible thing to do. Let's give it a couple of days to see if anything can be done on the groovy side before rolling our own.

@jeffscottbrown
Copy link
Member

I would advocate for having the default imports for Groovy in a Grails application to be the same as the default imports for Groovy outside of a Grails application.

@matrei
Copy link
Contributor

matrei commented Nov 7, 2024

I would advocate for having the default imports for Groovy in a Grails application to be the same as the default imports for Groovy outside of a Grails application.

Yes, I also thought about that. Maybe we should do it opt-in.

@codeconsole
Copy link
Contributor Author

@jeffscottbrown IMO I think it is a sensible default and should be adopted by Groovy. It is actually a hindrance to web applications to not have it because it promotes using legacy Date over time. This is of most use in gsp pages. We shouldn't be forcing developers to do date/time imports in gsp pages.

What would be the downside when there is already access to Date and Calendar?

@codeconsole
Copy link
Contributor Author

I also want to point out that the only reason I am strongly advocating this is because of how Date and Calendar are currently handled. If there wasn't already native support for those classes, I would not encourage start imports. I am not a big fan of star imports, but to successfully migrate a Grails application from Date/Calendar to the java.time.*, it is extremely painful having to deal with adding imports all over the place.

@codeconsole
Copy link
Contributor Author

Currently LocalDateTime does not render an input field.

@codeconsole
Copy link
Contributor Author

codeconsole commented Nov 9, 2024

IntelliJ IDE support is lacking, but more of nuisance as the project still builds and can be run with Gradle.

https://youtrack.jetbrains.com/issue/IDEA-362978/Groovy-Compiler-Path-to-configscript-does-not-work

@jdaugherty
Copy link
Contributor

jdaugherty commented Nov 10, 2024

Given that the imports could cause conflicts if Joda Time is still in use (older projects like grails projects are likely to have this happen), I'm of the opinion that we shouldn't default this to true. Given it's configurable, I don't feel too strongly about this though. I'd prefer to see this as an optional feature in 7.x and then default to true in 8.x. Especially if there is an intellij bug around the feature.

@codeconsole
Copy link
Contributor Author

codeconsole commented Nov 10, 2024

@jdaugherty what is the scenario where it could cause a conflict with Joda Time?

Clock
DateTimeException
DayOfWeek
Duration
Instant
InstantSource
LocalDate
LocalDateTime
LocalTime
Month
MonthDay
OffsetDateTime
OffsetTime
Period
Year
YearMonth
ZonedDateTime
ZoneId
ZoneOffset

6 to 7 is a MAJOR upgrade path. I don't think it is necessary to wait. A simpler path would be to detect if Joda time is in the class path and either automatically disable the behavior and give a warning or throw an exception and prompt the user to add the configuration to disable.

Please identify the scenario you think could cause a problem with people that use JodaTime.

@jeffscottbrown
Copy link
Member

We shouldn't be forcing developers to do date/time imports in gsp pages.

I agree with that. I don't think everyone agrees, but my opinion is that imports should never be in a GSP.

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 a pull request may close this issue.

5 participants