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

Add SystemUtil for querying common system properties #59

Merged
merged 5 commits into from
Feb 18, 2020

Conversation

clintval
Copy link
Member

@clintval clintval commented Oct 8, 2019

Functionality pulled from fgbio and dagr.

Which of these would you like as a part of public API?

@codecov-io
Copy link

codecov-io commented Oct 8, 2019

Codecov Report

Merging #59 into master will increase coverage by 0.28%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #59      +/-   ##
=========================================
+ Coverage   93.21%   93.5%   +0.28%     
=========================================
  Files          24      25       +1     
  Lines         825     831       +6     
  Branches       55      59       +4     
=========================================
+ Hits          769     777       +8     
+ Misses         56      54       -2
Impacted Files Coverage Δ
.../com/fulcrumgenomics/commons/util/SystemUtil.scala 83.33% <83.33%> (ø)
...ulcrumgenomics/commons/async/AsyncWriterPool.scala 100% <0%> (+6%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b83499e...9c0ad0b. Read the comment docs.

@nh13
Copy link
Member

nh13 commented Oct 8, 2019

@clintval what do you think about these changes? https://github.com/clintval/commons/pull/1. I'm happy to update fgbio and dagr for you.

@clintval
Copy link
Member Author

clintval commented Oct 8, 2019

A+ go for it.

Was going to write tests after the public API was revealed, but thanks for beating me to it!

@clintval
Copy link
Member Author

clintval commented Oct 9, 2019

@nh13, all set. I found a way to have anchored regex matching that is 2.12 and 2.13 compatible. Ping me if you want help with the rest.

Copy link
Member Author

@clintval clintval left a comment

Choose a reason for hiding this comment

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

I would be interested in seeing this merged as I want to factor this into PicardTask in Dagr.

Copy link
Member

@nh13 nh13 left a comment

Choose a reason for hiding this comment

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

One suggested change you yourself suggested.

@clintval
Copy link
Member Author

clintval commented Jan 13, 2020

Few changes:

  • Copyrights to 2020
  • Changed getSystemProperty to safePropOrNone to match the ecosystem of Scala property getters in scala.util.Properties for when you want to get a system property safely (catch security manager exceptions)
  • Deleted lazy val OsName as that's in the Scala builtins
  • Moved over implementations to other Scala builtins like isLinux, isMac, and propOrNone

@clintval
Copy link
Member Author

@nh13 I think this is merge-ready FYI

@clintval clintval removed their assignment Jan 31, 2020
@clintval
Copy link
Member Author

clintval commented Feb 9, 2020

If possible, I'd like to find a way to move this along because I also want to use these features for patches to fgbio and dagr. I'm still having difficulty with the occasional core dump on MacOS (10.15.3) due to the Intel infalters/deflaters and want to unblock fulcrumgenomics/dagr#368.

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.

3 participants