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

gh-501: change imports to be directly from glass #512

Merged
merged 4 commits into from
Feb 14, 2025
Merged

Conversation

paddyroddy
Copy link
Member

Description

Changes the code throughout to use import glass rather than from glass import where possible. This is possible via the __all__ statements in __init__.py. I have also done this for glass.grf.

Closes: #501

Changelog entry

Checks

  • Is your code passing linting?
  • Is your code passing tests?
  • Have you added additional tests (if required)?
  • Have you modified/extended the documentation (if required)?
  • Have you added a one-liner changelog entry above (if required)?

@paddyroddy paddyroddy added the maintenance Maintenance: refactoring, typos, etc. label Feb 13, 2025
@paddyroddy paddyroddy self-assigned this Feb 13, 2025
from glass.grf._core import corr, dcorr, icorr
from glass.grf import corr, dcorr, icorr
Copy link
Member Author

Choose a reason for hiding this comment

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

This one has to remain from glass.grf syntax as otherwise we run into a circular import

Comment on lines -28 to +27
Fields = Sequence[grf.Transformation]
Fields = Sequence[glass.grf.Transformation]
Copy link
Member Author

Choose a reason for hiding this comment

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

This was incorrect before

@paddyroddy paddyroddy marked this pull request as ready for review February 13, 2025 17:54
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

@ntessore
Copy link
Collaborator

ntessore commented Feb 13, 2025

Thanks for doing all those changes!

I hadn't really thought about use of glass from inside the library itself, I was only considering tests and examples as "uses" of glass. Do you then prefer to use the glass namespace everywhere, even in library code itself? I have no opinion either way.

@paddyroddy
Copy link
Member Author

Do you then prefer to use the glass namespace everywhere, even in library code itself? I have no opinion either way.

I don't really mind. I think there are benefits of both.

The way I've done it means that you are using the library as an external user would. So in a sense it's another check that __all__ etc. is working.

On the other hand, having full path imports means that at a glance you know exactly where something is coming from. This is why I in general prefer import <x>; <x>.<y>, rather than from <x> import <y>, as at a glance I wouldn't know where <y> has come from.

@Saransh-cpp do you have any views of which style we should use here?

@Saransh-cpp
Copy link
Member

I like import <x>; <x>.<y> as well for the same reasons.

Also, all the popular conventions in the Python ecosystem are always of the form import <x>; <x>.<y> -

numpy.array
plt.plot
pd.DataFrame
...

@ntessore
Copy link
Collaborator

Ok, then let's always use glass. even in GLASS itself. I guess that doesn't cause any more circular import grief than the from ... import ... methods, since latter also imports glass first. LGTM!!

@paddyroddy paddyroddy merged commit e66418a into main Feb 14, 2025
16 checks passed
@paddyroddy paddyroddy deleted the paddy/issue-501 branch February 14, 2025 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance: refactoring, typos, etc.
Projects
None yet
3 participants