-
Notifications
You must be signed in to change notification settings - Fork 30
chore: Sort imports to prefer module-level imports to function-level imports #227
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
paleolimbot
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.
There are a few of these that should stay put but in general this is a helpful change...thank you for taking this on!
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.
The files in sedona-geo-generic-alg are mostly copied from another source. I think moving around the imports may make it difficult to track updates to the upstream code.
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.
Thought as much, I've reverted all changes, and restored it to its initial state
| use ScalarValue::*; | ||
|
|
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 should probably stay here...the wildcard imports from enums are often best kept local because they may bring a lot of common names into scope!
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 has been restored as well
|
|
||
| impl Display for GeoParquetColumnEncoding { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| use GeoParquetColumnEncoding::*; |
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 one should also probably stay here (it is a wildcard import of an enum that brings a lot of names into scope)
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.
Same here, I've restored this
| use std::env; | ||
| use std::path::MAIN_SEPARATOR; |
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.
These should probably stay here: function level imports under a [cfg(...)] help prevent clippy errors about unused imports when compiling on a different platform.
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.
Yeah, I've restored
|
My Bad, I didn't crosscheck |
paleolimbot
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.
Thank you!
Thi Pull Request aims to address the issue where imports are duplicated, not properly sorted, Omitting only
rust/sedona-geo-generic-alg/src/algorithm/line_measures/metric_spaces/euclidean/utils.rswhich the import choices at the tail tests seems intentional.