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

Default import of java.time.* #346

Merged
merged 7 commits into from
Nov 9, 2024
Merged

Conversation

codeconsole
Copy link
Contributor

@codeconsole codeconsole commented Nov 3, 2024

Enables smooth transition from legacy java.util.Date to java.time

Fixes #345

@@ -652,6 +654,31 @@ class GrailsGradlePlugin extends GroovyPlugin {
output?.classesDirs ?: project.files(new File(project.buildDir, "classes/main"))
}

protected void configureJavaTime(Project project) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is autoImportDateTimeApi(Project project) a more descriptive name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn't just auto importing DateTime. I think it is importing java.time.*. Is that the case? If it is, I don't think autoImportDateTimeApi is a good name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, autoImportDateTimeApi is at least the sentiment of what we are trying to do. There are of course a couple of sub-packages that also belong to the DateTime Api.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand now. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matrei my sentiment was always just import java.time.* and that is why I named it JavaTime. Are you suggesting we should import everything else?

java.time.chrono.*
java.time.format.*
java.time.temporal.*
java.time.zone.*

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting we should import everything else?

I don't know. Are there any drawbacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if it is necessary or needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the groovy examples only needed:

import java.time.*
import static java.time.temporal.ChronoField.*
import java.time.temporal.ChronoUnit
import java.time.format.FormatStyle

to work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matrei drawback is that I am not a fan of start imports to begin with, but I feel we need to offer a smooth painless transition away from the java Date class.

@@ -654,31 +674,6 @@ class GrailsGradlePlugin extends GroovyPlugin {
output?.classesDirs ?: project.files(new File(project.buildDir, "classes/main"))
}

protected void configureJavaTime(Project project) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I meant to keep this method (although with a different name), and call it from the configureGroovy method.

protected void configureGroovy(Project project) {
     // some other stuff

    if (grailsExt.autoImportDateTimeApi) {
        autoImportDateTimeApi(project)
    }
}

@codeconsole
Copy link
Contributor Author

codeconsole commented Nov 9, 2024

I am going to merge this. I am not opposed to any changes. I just want to keep things moving. The discussion can continue and this can continue to evolve, but there are other areas right now where this needs to be expanded on and the work can not continue without the merge. For instance, LocalDateTime currently does not render an input. Let's continue discussion in #345

@codeconsole codeconsole merged commit 4b11413 into grails:7.0.x Nov 9, 2024
6 checks passed
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.

java.time needs to be better supported in Grails 7
3 participants