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

Switched to session as parameter for the fs #11

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Switched to session as parameter for the fs #11

merged 4 commits into from
Nov 29, 2023

Conversation

hechth
Copy link
Owner

@hechth hechth commented Nov 28, 2023

Closes #9

raw_info["basic"]["name"] = data_object.name
raw_info["access"]["user"] = data_object.owner_name

raw_info["details"]["modified"] = data_object.modify_time.timestamp()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if using timestamp( ), we have to be careful since iRODS modify times are UTC.
Observe that running on my computer (EST timezone) I get

>>> datetime.datetime(1970,1,1,0,0).timestamp()
18000.0

But:

>>> utc=datetime.timezone(datetime.timedelta(0))
>>> datetime.datetime(1970,1,1,0,0).replace(tzinfo=utc).timestamp()
0.0

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah so then it makes sense to always convert them to UTC.

Copy link
Collaborator

@d-w-moore d-w-moore Nov 29, 2023

Choose a reason for hiding this comment

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

Convert in a matter of speaking yes. Although most people would take "conversion" as a translation from one timezone to another; and we're instead making a naive datetime object into a localized one of the proper timezone, ie UTC.

Copy link
Collaborator

@d-w-moore d-w-moore left a comment

Choose a reason for hiding this comment

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

If these work, I think it's a solid change - with the idea that once the session is given to the pyfilesystem object, it is owned by that object and shouldn't be used outside.

@hechth
Copy link
Owner Author

hechth commented Nov 29, 2023

The main point is that otherwise returning files as is expected by the open and openbin functions becomes very difficult as the session is closed eventually before the file.

Right - the main idea being that our fs(iRODSFS) object should hold a reference to the session instance, thus preventing it from going out of scope for the lifetime of the pyfilesystem connection. It's a side benefit, I think, by virtue of your changes here, that the iRODS session may be created in any way the user desires. A side-effect is that the iRODSFS object will also need to be kept around for the open file descriptor's benefit, but this might be considered de rigeur anyway. To help enforce the idea, we might document that, as well as maybe give the iRODSFS an _irods_session_key method so that users can easily get that value for URL-style access. Or, if I'm off-base, please tell me.

@hechth hechth merged commit cf10722 into main Nov 29, 2023
1 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement opening files with context manager or raw?
2 participants