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

Please use java.time.LocalDate et al, instead of org.threeten.bp.* #274

Open
rubixcubez opened this issue Mar 24, 2021 · 7 comments
Open
Labels
Status: On Hold Provide an explanation in the comments Type: Feature Request

Comments

@rubixcubez
Copy link

As per title. The vast majority of people using this library would hopefully be on Java 1.8+, which means access to java.time.LocalDate etc. Having to bring in an external library just so that I can programmatically add 21 days to an invoice date is a bit painful. I know the setDueDate(String) alternative is available, but it should not be necessary.
The below doesn't work -
invoice.setDueDate(myLocalDate.plusDays(DUE_DATE_DAYS_AFTER_INVOICE_DATE));

@SidneyAllen
Copy link
Contributor

Hi @rubixcubez - thanks for the feedback.

We will consider this request in the future. At this time, it would be a major breaking change, so we will evaluate it along with other requests that would require significant refactoring of existing integrations. I'll keep this issue open for future reference.

@SidneyAllen SidneyAllen added Status: On Hold Provide an explanation in the comments Type: Feature Request labels Apr 7, 2021
@lancedfr
Copy link
Contributor

lancedfr commented May 11, 2021

I so agree (insert crying face) I actually started work on this 3 months ago here but never finished testing it.

The use of org.threeten.bp dates is so tedious we were considering publishing a forked version of the sdk ourselves and keep up to date with your version of the sdk by grafting changes across every release :(

@SidneyAllen, this change will help us. I'm happy if you want to assign this to me I can wrap up this feature in the above mentioned fork.

@SidneyAllen
Copy link
Contributor

@lancedfr - Thanks for the offer. Since our SDKs are auto-generated from templates, we'll need to update the templates for this change. I anticipate this being done in conjunction with other large breaking changes that will take the SDK from 4.x to 5.x

@rocketraman
Copy link

rocketraman commented Dec 1, 2021

This is pretty dumb. Java 7 (the last version on which anyone would need the threeten libraries) has been end of life since April 2015.

@SidneyAllen wrote:

We will consider this request in the future. At this time, it would be a major breaking change, so we will evaluate it along with other requests that would require significant refactoring of existing integrations. I'll keep this issue open for future reference.
...
I anticipate this being done in conjunction with other large breaking changes that will take the SDK from 4.x to 5.x

Umm, no, this is the wrong approach. What you should do is add support for java.time types now without breaking any existing API. For example, you could publish overrides of each method that takes a threeten date, and take a java.time value instead. You then mark the old threeten methods as deprecated with a note that they are scheduled to be removed in the next major version.

People will start getting warnings in their code, but nothing will break, giving them time to migrate their code base. The next major release then removes the threeten methods.

If you wait until the next major release and suddenly change everything to java.time from threeten, you provide your users with no ability to plan their migration path whatsoever. They'll be stuck on the old version of the SDK until they can spend the time to update their code all at once.

@fprochazka
Copy link

fprochazka commented Mar 3, 2023

For anyone having a problem with this: the threeten library defines the exactly same types as java.time (or at least this lib is not using anything that wouldn't also be in java.time), so I've created a maven module, that downloads the lib, patches it and lets my app use the patched version...

Basically all you have to do is replace the package prefix org.threeten.bp. -> java.time.

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-dependency-plugin</artifactId>
    <version>${maven-dependency-plugin.version}</version>
    <inherited>false</inherited>
    <executions>
        <execution>
            <id>unpack-xero</id>
            <phase>generate-sources</phase>
            <goals>
                <goal>unpack</goal>
            </goals>
            <configuration>
                <artifactItems>
                    <artifactItem>
                        <groupId>com.github.xeroapi</groupId>
                        <artifactId>xero-java</artifactId>
                        <version>${xero-api.version}</version>
                        <type>jar</type>
                        <classifier>sources</classifier>
                        <outputDirectory>${project.build.directory}/generated-sources/patched</outputDirectory>
                        <includes>com/xero/models/**/*.java,com/xero/api/StringUtil.java</includes>
                    </artifactItem>
                </artifactItems>
                <overWriteIfNewer>true</overWriteIfNewer>
                <overWriteReleases>false</overWriteReleases>
                <overWriteSnapshots>false</overWriteSnapshots>
            </configuration>
        </execution>
    </executions>
</plugin>

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-antrun-plugin</artifactId>
    <version>${maven-antrun-plugin.version}</version>
    <executions>
        <execution>
            <phase>process-sources</phase>
            <goals>
                <goal>run</goal>
            </goals>
            <configuration>
                <target>
                    <replace token="org.threeten.bp." value="java.time." dir="${project.build.directory}/generated-sources/patched">
                        <include name="**/*.java"/>
                    </replace>
                </target>
            </configuration>
        </execution>
    </executions>
</plugin>

this is not the full POM, but the rest won't help you much because my build is probably a lot different from yours

@lancedfr
Copy link
Contributor

lancedfr commented Mar 3, 2023

@fprochazka we are doing the same, but in a new project. We package and publish the new project to our own nexus server for our developers to use.

One reason I didnt post this solution is because I dont want it to be the solution everyone implements, it should be implemented in Xero-Java.

With that said, do you come across any issues with your fix? We haven't, which could stand as evidence to Xero that a simple find/replace will work. It has worked for us and I assume for you to 👍

@fprochazka
Copy link

It works for me without any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: On Hold Provide an explanation in the comments Type: Feature Request
Projects
None yet
Development

No branches or pull requests

5 participants