-
Notifications
You must be signed in to change notification settings - Fork 121
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
Normalize import style #1259
Comments
Ok I have thoughts. For internal usage I think we should import the minimum and from the file where the definition is. So I'd prefer: from pystac.utils import str_to_datetime
from pystac.item import Item
item = Item.from_path("item.json")
datetime_str = str_to_datetime("2023-10-12T09:00:00Z") For external usage (I include tests in this) I went to see what pep8 says. It is pretty generous:
So for pystac, I agree that the big top-level classes (Asset, Item, Collection, ItemCollection, Catalog) should all be ok to directly import but then I look at pystac-client and I want to follow the same best practices but I really don't want confusion between |
I'd be ok with this -- issues can arise if two modules have functions with the same name, but I think PySTAC's currently built so that's not really a problem. I'll 👍🏼 this idea.
Agreed, and I don't think we need to recommend how people use PySTAC -- I was mostly focusing on the interal usage. |
* refactor: normalize import style #1259 * refactor(tests): modernize version extension tests - #993 - #948 * feat: add experimental to version ext * feat: version is optional in version extension * refactor: test import naming, ext * feat: catalog extensions * refactor: import style * feat: add history media type * feat: separate BaseVersionExtension * feat: impl migration * doc: fixups * chore: update changelog * fix(tests): mark some extra vcrs * fix: pop_if_none on deprecated
@gadomski prefers importing classes directly, but not functions. E.g.
Happy to arguments for other syles, but once we settle on one, we should normalize the repo. Maybe there's an automated tool to do this? If not, it'll be some manual choreing.
The text was updated successfully, but these errors were encountered: