Skip to content

Conversation

palindrom615
Copy link

According to git document, git is using unique datetime format with --iso option.

Replacing it with strict ISO8601 format will improve usability.

@CLAassistant
Copy link

CLAassistant commented Feb 15, 2024

CLA assistant check
All committers have signed the CLA.

@ikhoon
Copy link
Contributor

ikhoon commented Feb 29, 2024

Replacing it with strict ISO8601 format will improve usability.

@palindrom615 May I know what usability is improved? For humans, --iso would be better for readability.
I am asking because this change could break the parsing logic of versions.properties in Armeria.
https://github.com/line/armeria/blob/2e99b180518f2543c9189a1fe00d5a711a4a7a49/core/src/main/java/com/linecorp/armeria/common/util/Version.java#L202-L204

@palindrom615
Copy link
Author

@ikhoon I see. that should not break anything. WDYT about adding commitISODate property as in 6b12c4e?

@palindrom615 May I know what usability is improved? For humans, --iso would be better for readability.

It saves some codes for parsing dateformat. For instance, jib uses iso8601.

jib {
    ...

    container {
        // depends on `common-git.gradle`
        if (
            project.ext.has("repoStatus") &&
            (project.ext.get("repoStatus") as Map<String, Any>)["repoStatus"] as String == "clean"
        ) {
            val repoStatus = project.ext.get("repoStatus") as Map<String, String>
            val commitISODate = repoStatus.get("commitISODate")
            creationTime.set(commitISODate)
        }
    }
}

@ikhoon
Copy link
Contributor

ikhoon commented Mar 15, 2024

WDYT about adding commitISODate property as in 6b12c4e?

Sounds good. I think we can keep both of them for a while and then deprecate and remove commitDate when all usages are migrated.

Would you be interested in sending a PR to Armeria as well when this PR is merged?

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks, @palindrom615!

@palindrom615
Copy link
Author

Would you be interested in sending a PR to Armeria as well when this PR is merged?

sure I can 🙇

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Sounds like a good idea. Thanks, @palindrom615 🙇

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks, @palindrom615!

@minwoox
Copy link
Contributor

minwoox commented Jun 10, 2024

@jrhee17 Do you mind reviewing this PR? >_<

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.

5 participants