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

Feature/issue 123 #129

Merged
merged 1 commit into from
Mar 9, 2021
Merged

Feature/issue 123 #129

merged 1 commit into from
Mar 9, 2021

Conversation

brbrown25
Copy link
Contributor

No description provided.

@brbrown25
Copy link
Contributor Author

@satabin sorry for the delay on this. I've split it into two commits, one with the changes and another where I apply scalafmt to all the things. I noticed this when I ran scalafmt as part of the process that a bunch were out of format. If you'd like I can remove that commit. I also went ahead and added in the other java.time classes just for completeness. Thanks so much for the opportunity and this awesome project! I'm currently using it at work and it's been a huge help.

@brbrown25
Copy link
Contributor Author

ohh looks like a test I added broke, gonna look at that right now

@satabin
Copy link
Member

satabin commented Mar 9, 2021

Hello @brbrown25,

Thanks a ton for the contribution, I will go ahead and review it tonight. Regarding formatting, it might be that master is not properly formatted, if that's the case, I'll do it directly on master, and add a PR check to ensure master is always formatted in the future. You don't need to do it in this PR.

I am super glad to read this project is useful for your work and hope it will continue fitting your use cases.

@brbrown25 brbrown25 force-pushed the feature/ISSUE-123 branch 2 times, most recently from 47dc50b to 3c21ec7 Compare March 9, 2021 14:59
@brbrown25
Copy link
Contributor Author

@satabin it looks like scala js only shows UTC and GMT as zones, hence the failing test. to get around this I added a js and jvm version rather than having them in shared. it seems slightly related to cquiroz/scala-java-time#257 which I might try and look at later this week

@brbrown25 brbrown25 force-pushed the feature/ISSUE-123 branch from 442e374 to 0fef6a4 Compare March 9, 2021 15:30
@brbrown25
Copy link
Contributor Author

Update, turns out if I add in the time-zone db dependency my original issue with the js test failing goes away, so I've updated with that.

Copy link
Member

@satabin satabin left a comment

Choose a reason for hiding this comment

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

Looks good to me, a minor improvement, and it should be good to be merged.

@brbrown25 brbrown25 force-pushed the feature/ISSUE-123 branch from 0fef6a4 to 672261a Compare March 9, 2021 21:35
@brbrown25 brbrown25 requested a review from satabin March 9, 2021 21:35
Copy link
Member

@satabin satabin left a comment

Choose a reason for hiding this comment

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

LGTM

@satabin satabin merged commit ba6a2a9 into gnieh:master Mar 9, 2021
@brbrown25 brbrown25 deleted the feature/ISSUE-123 branch March 9, 2021 23:18
@satabin satabin added this to the 0.10.0 milestone Mar 20, 2021
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