Skip to content

clib.session: Refactor to simplify the checking of currently open session #3523

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.

Sign up for GitHub

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

Merged
seisman merged 3 commits into from
Oct 16, 2024
Merged

clib.session: Refactor to simplify the checking of currently open session #3523

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d0c91b
clib.session: Refactor to simplify the checking of currently open ses…
seisman Oct 16, 2024
da9c5e1
Revert back to try-except
seisman Oct 16, 2024
1725ea9
Update pygmt/clib/session.py
seisman Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions pygmt/clib/session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def session_pointer(self) -> ctp.c_void_p:
If trying to access without a currently open GMT session (i.e., outside of
the context manager).
"""
if not hasattr(self, "_session_pointer") or self._session_pointer is None:
if getattr(self, "_session_pointer", None) is None:
Copy link
Member Author

@seisman seisman Oct 16, 2024

Choose a reason for hiding this comment

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

if not hasattr(self, "_session_pointer") or self._session_pointer is None: is equivalent to if getattr(self, "_session_pointer", None) is None:.

raise GMTCLibNoSessionError("No currently open GMT API session.")
return self._session_pointer

Expand Down Expand Up @@ -338,17 +338,16 @@ def create(self, name: str):
name
A name for this session. Doesn't really affect the outcome.
"""
Copy link
Member Author

@seisman seisman Oct 16, 2024

Choose a reason for hiding this comment

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

If self.session_pointer is defined and is not None, then we know we have an open session. The try..except statement was added in 2018 (#210) and is not necessary.

Copy link
Member

@weiji14 weiji14 Oct 16, 2024

Choose a reason for hiding this comment

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

As mentioned in #3448 (comment), a with contextlib.supress block should ideally be used when there is only one line below it. I feel like we should keep the previous try-except block.

Copy link
Member Author

@seisman seisman Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's impossible that the msg assignment and the raise GMTCLibError lines can raise a GMTCLibNoSessionError exception, so we should be safe.

Edit: Read astral-sh/ruff#1947 again, actually it's unclear if the raise GMTCLibError statement will be executed or not. I think we have to go back to try..except.

Copy link
Member Author

@seisman seisman Oct 16, 2024

Choose a reason for hiding this comment

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

Reverted back to try..except in da9c5e1 and improve the comments to make it easier to understand.

# Check if there is a currently open session by accessing the "session_pointer"
# attribute. If not, it will raise the GMTCLibNoSessionError exception and we're
# free to create a new one. Otherwise, we will raise a GMTCLibError exception.
try:
# Won't raise an exception if there is a currently open session.
_ = self.session_pointer
# In this case, fail to create a new session until the old one is destroyed.
msg = (
"Failed to create a GMT API session: There is a currently open session."
" Must destroy it first."
"Failed to create a GMT API session: "
"There is currently an open session. Must destroy it first."
)
raise GMTCLibError(msg)
# If the exception is raised, this means that there is no open session and we're
# free to create a new one.
except GMTCLibNoSessionError:
pass

Expand Down