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

New timestamp format using BigDecimal and Instant #1313

Merged

Conversation

adamsero
Copy link
Contributor

No description provided.

@sbernard31
Copy link
Contributor

@adamsero, looking at your PR, I see that github action does not work as expected.
When error is detected, comments should be added to PR to help you to resolve it. This should like this sbernard31#12.

I create #1314 to try to resolve it.

@adamsero adamsero force-pushed the opl/new_timestamp_format branch from e31893b to 5204ac9 Compare September 19, 2022 14:09
@adamsero
Copy link
Contributor Author

@sbernard31 regarding your comment, specifically

Could you not fix the last one about Check Android API Compliance ?

I'm getting a bunch of errors caused by the use of Instant class, since it's not compatible with Android API v19 as defined in build-config/lib-build-config/pom.xml:

<signature>
    <groupId>net.sf.androidscents.signature</groupId>
    <artifactId>android-api-level-19</artifactId>
    <version>4.4.2_r4</version>
</signature>

The fact that we check against API v19 released in 2013 means that we can't use any feature of Java 1.8 (from 2014), so we either have to bump the API version (I don't know what issues could arise from that) or this PR stays useless since I can't use the Instant class.

@sbernard31
Copy link
Contributor

sbernard31 commented Sep 19, 2022

About Undefined reference: java.time.Instant :

Sorry I didn't think to check if this was available.
Strange you didn't face this issue sooner. I guess you don't often run maven locally ?

I continue the discussion at #1304

@sbernard31
Copy link
Contributor

After discussion at #1304 (comment).

We decide to bump android API level from 19 to 26.
So from

<signature>
    <groupId>net.sf.androidscents.signature</groupId>
    <artifactId>android-api-level-19</artifactId>
    <version>4.4.2_r4</version>
</signature>

to something like

<signature>
    <groupId>net.sf.androidscents.signature</groupId>
    <artifactId>android-api-level-26</artifactId>
    <version>8.0.0_r2</version>
</signature>

@adamsero
Copy link
Contributor Author

We should be good now @sbernard31, waiting for you to bump the API version if you don't have any more issues to fix.

@adamsero
Copy link
Contributor Author

adamsero commented Oct 4, 2022

@sbernard31 I created some tests and added some exception throws in the util method. My question is, can we possibly be done with this PR and M9 by the end of the week? It would be really good timing if we could.

@sbernard31
Copy link
Contributor

My question is, can we possibly be done with this PR and M9 by the end of the week? It would be really good timing if we could.

We can try.

@sbernard31
Copy link
Contributor

I think we are good now ?

I let you deal with Maven / Code Check github action and upgrade the Android level API.

Then maybe you could rebase on `master and, squash all commits in one ? (then push force)

Eventually you can Android level API change in a separated commit, if you do that then this android level api change commit should be before the modification. (but again it's ok if you squash all in 1)

Last point, maybe you can refer #1304 issue in your commit header.

@adamsero adamsero force-pushed the opl/new_timestamp_format branch from 024f4f7 to 2fdf9f1 Compare October 5, 2022 22:00
@adamsero
Copy link
Contributor Author

adamsero commented Oct 5, 2022

Should be good to go. Good to finally see a green checkmark 🙂

@sbernard31 sbernard31 merged commit 2fdf9f1 into eclipse-leshan:master Oct 6, 2022
@sbernard31
Copy link
Contributor

Thx for the contribution 🙏 !

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.

2 participants