Added handling for NAN in add method of DateTimeStamp #50

anantk24 · 2021-08-18T16:49:47Z

When offsetInDecimalSeconds is NAN then it is considered as 0.0d
DateTimeStamp API that takes double as an arg should handle NaN #49

- Defined hash code api - added null check and class check in equals api - Modified access modifiers according to JLS.

…ogs. - Updated command in maven github workflow. - Added default and contributor profile - Updated read me doc and contributing doc.

Minor edit for punctuation.

`mvn test` will still run, but does nothing. This would be surprising to anyone running this build. So I added back the `mvn test` line with some info about the contributor profile.

- When offsetInDecimalSeconds is NAN then it is considered as 0.0d

dsgrieve · 2021-08-18T20:45:33Z

core/api/src/test/java/com/microsoft/gctoolkit/test/time/DateTimeStampTest.java

+    @Test
+    void testNanWithNegativeAdd() {
+        DateTimeStamp dateTimeStamp = new DateTimeStamp("2018-04-04T10:09:59.586-0100");
+        DateTimeStamp forComparing = new DateTimeStamp("2018-04-04T10:10:00.586-0100").add(-1);


shouldn't this be minus(Double.NaN)?

dsgrieve · 2021-08-18T21:10:30Z

Would you mind looking at all of the DateTimeStamp API that takes a double?

There is a hole in the constructor where new DateTimeStamp((String)null, Double.NaN) sets timeStamp = Double.NaN
before/after(Double.NaN) both return false (because they compare with inequality). Double.compareTo handles NaN, but if we interpret NaN as zero, then it doesn't do the right thing. I mean, Double.compareTo(42d, Double.NaN) returns -1 where we want Double.compareTo(42d, 0.0d) (which returns 1).

anantk24 · 2021-08-19T10:50:31Z

Would you mind looking at all of the DateTimeStamp API that takes a double?

There is a hole in the constructor where new DateTimeStamp((String)null, Double.NaN) sets timeStamp = Double.NaN

before/after(Double.NaN) both return false (because they compare with inequality). Double.compareTo handles NaN, but if we interpret NaN as zero, then it doesn't do the right thing. I mean, Double.compareTo(42d, Double.NaN) returns -1 where we want Double.compareTo(42d, 0.0d) (which returns 1).

Yes sure will look for all api.

…dded testcases for the same.

anantk24 · 2021-08-23T15:43:45Z

@dsgrieve @karianna @pliakas Any guess why this is failing ?
My branch is up to date. I haven't changed any file in workflow, looks like some step from my side is missing.
I am not able to understand the root cause?
Any suggestion? Thanks !

dsgrieve · 2021-08-23T17:37:03Z

@anantk24 It's looking for a file DateTimeStampNaN which it can't find. Is this a file you created? If so, it was never pushed.

anantk24 · 2021-08-24T06:36:21Z

@anantk24 It's looking for a file DateTimeStampNaN which it can't find. Is this a file you created? If so, it was never pushed.

It is the branch I have created , trying to create pull request from the same.
commits into microsoft:main from anantk24:DateTimeStampNAN
I tried creating different pull request from diff branch #54 . It failed there too.

dsgrieve · 2021-08-24T11:29:09Z

It looks like the build script is trying to commit the badges to gctoolkit and not your fork, which I guess makes sense. Maybe there needs to be a different script for PRs that doesn't do the badging. Or maybe there is some config for the commit script that's missing. IDK. New territory for me. I need to investigate.

dsgrieve · 2021-08-24T13:03:27Z

@anantk24 - I pushed a fix for the badge commit. Please pull from gctoolkit/main

anantk24 · 2021-08-24T13:41:29Z

It looks like the build script is trying to commit the badges to gctoolkit and not your fork, which I guess makes sense. Maybe there needs to be a different script for PRs that doesn't do the badging. Or maybe there is some config for the commit script that's missing. IDK. New territory for me. I need to investigate.

@dsgrieve - Thanks for update. I was thinking on similar lines, opened discussion #52 for the same. Automatic badge update is available in markdown and through supported code analysis server.

anantk24 · 2021-08-24T13:46:38Z

@anantk24 - I pushed a fix for the badge commit. Please pull from gctoolkit/main

@dsgrieve Thanks for changes. Pulled changes from main. It failed again . Is it working for everyone else?

dsgrieve · 2021-08-24T13:46:55Z

I'm not sure why this worked for my PR and not yours, unless it has to do with some mojo I have that you do not. Maybe committing the badge doesn't make sense for a PR build. But the git-auto-commit-action doc implies it should work for the PR. Maybe it's the detached head state of the checkout. Maybe the workaround is to have a separate PR workflow yaml.

dsgrieve · 2021-08-24T14:47:06Z

I just recreated the issue by checking out your pr, setting upstream to my fork, and then creating a PR from there (PR #57).

The reason it happens on your branch and not others is that you added tests, thus changing the badge, resulting in an attempt to do the commit.

The answer, I believe, lies somewhere in the branch option of the git-auto-commit-action.

…ampNAN

dsgrieve · 2021-08-24T18:01:26Z

Thanks again, @anantk24!

anant and others added 11 commits August 16, 2021 21:25

Solved spotbugs related to GCEvent class.

583e865

- Defined hash code api - added null check and class check in equals api - Modified access modifiers according to JLS.

Merge branch 'microsoft:main' into main

fe5e22e

Added contributor profile for executing testcases and download test l…

f57ad74

…ogs. - Updated command in maven github workflow. - Added default and contributor profile - Updated read me doc and contributing doc.

Merge branch 'main' into main

eb9254f

Update CONTRIBUTING.md

d309b1c

Minor edit for punctuation.

Update README.md

7824e78

`mvn test` will still run, but does nothing. This would be surprising to anyone running this build. So I added back the `mvn test` line with some info about the contributor profile.

Merge branch 'microsoft:main' into main

823dfee

Added handling for NAN in add method of DateTimeStamp

53159d1

- When offsetInDecimalSeconds is NAN then it is considered as 0.0d

Added handling for NAN in add method of DateTimeStamp

674f034

- When offsetInDecimalSeconds is NAN then it is considered as 0.0d

Merge remote-tracking branch 'origin/main' into microsoft-main

87eafb5

Merge branch 'microsoft-main' into DateTimeStampNAN

55f651a

anantk24 mentioned this pull request Aug 18, 2021

DateTimeStamp API that takes double as an arg should handle NaN #49

Closed

dsgrieve reviewed Aug 18, 2021

View reviewed changes

karianna added the bug Something isn't working label Aug 19, 2021

Added function for getTimestampValue for evaluating NAN as zero and a…

e6b4c0b

…dded testcases for the same.

Merge branch 'microsoft:main' into DateTimeStampNAN

dab867c

anantk24 mentioned this pull request Aug 24, 2021

Added function getTimestampValue to handle nan value . #54

Closed

Merge branch 'microsoft:main' into DateTimeStampNAN

bb9c9eb

Merge branch 'main' of github.com:microsoft/gctoolkit into DateTimeSt…

5317b3e

…ampNAN

dsgrieve approved these changes Aug 24, 2021

View reviewed changes

dsgrieve merged commit 2f379de into microsoft:main Aug 24, 2021

Added handling for NAN in add method of DateTimeStamp #50

Added handling for NAN in add method of DateTimeStamp #50

Uh oh!

Conversation

anantk24 commented Aug 18, 2021

Uh oh!

dsgrieve Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

dsgrieve commented Aug 18, 2021

Uh oh!

anantk24 commented Aug 19, 2021

Uh oh!

anantk24 commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsgrieve commented Aug 23, 2021

Uh oh!

anantk24 commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsgrieve commented Aug 24, 2021

Uh oh!

dsgrieve commented Aug 24, 2021

Uh oh!

anantk24 commented Aug 24, 2021

Uh oh!

anantk24 commented Aug 24, 2021

Uh oh!

dsgrieve commented Aug 24, 2021

Uh oh!

dsgrieve commented Aug 24, 2021

Uh oh!

dsgrieve commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anantk24 commented Aug 23, 2021 •

edited

Loading

anantk24 commented Aug 24, 2021 •

edited

Loading