-
Notifications
You must be signed in to change notification settings - Fork 256
[query] clean-up unused fs methods #15177
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
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
38bd775 to
7ea3e1a
Compare
chrisvittal
left a comment
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.
this is fantastic
| val account: String, | ||
| val container: String, | ||
| val path: String, | ||
| override val path: String, |
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.
Is this override for clarity? It's not necessary.
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.
Could you explain why it's not necessary?
For clarity and better tool support. I've gotten into the habit of adding override to any member that overrides the superclass as intellij's tooling is much better at refactoring/renaming these. In the past when I've renamed/added or removed params/moved it didn't apply the changes to members not marked override which lead to complile errors and time wasting.
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've long wanted a way to enforce using the override keyword, as I agree with Ed about the benefits, but it's very hard to remember to add it every time without tooling support. This comment made me do another look around, and I think I found a way to do that. I'll give it a try shortly.
7ea3e1a to
7bdccf4
Compare

Change Description
This PR refactors the filesystem abstraction layer to improve the URL handling interface. The main changes include:
FSURLfrom a trait to an abstract class with a generic type parameter/to replaceaddPathComponentreadNoCompression,deleteOnExit,stripCodecExtension, and caching-related methodsgetPathto simplypathfor cleaner accessThese changes make the filesystem API more consistent and easier to use while removing unnecessary functionality.
Security Assessment
This change cannot impact the Hail Batch instance as deployed by Broad Institute in GCP