-
Notifications
You must be signed in to change notification settings - Fork 175
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
Remove java.io in FileUtils #2256
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGMT, just need deprecation annotations.
af31d93
to
58c6479
Compare
LGTM, this is the first step to remove complex |
See you in your next PR @sinofp 👍 |
/** Get os.Path from String | ||
* @param pathName an absolute or relative path string | ||
*/ | ||
def getPath(pathName: String): os.Path = os.FilePath(pathName) match { | ||
case path: os.Path => path | ||
case sub: os.SubPath => os.pwd / sub | ||
case rel: os.RelPath => os.pwd / rel | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit late here, but it seems this is enshrining use of absolute paths which I understand is convenient but is actually generally not a great thing to do since it makes runs not relocatable (eg. if I emit files with absolute paths in them, then the one in my workspace is different than the one in your work space).
To be fair, I'm not sure if the existing APIs are already very "absolutely path centric" but it's generally something we should move away from rather than encouraging.
@sinofp , @sequencer what do I need to upgrade to get this |
Form these lines: Line 39 in de56a19
Line 51 in de56a19
I think os-lib is available in the latest SNAPSHOT(master), can you DM me your build flow?
|
I got past that by adding the com:lihayi library to my build, thanks! On to the next thing... |
haha, I guess it blames to |
Contributor Checklist
Type of Improvement
API Impact
Backend Code Generation Impact
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
Please Merge
?