Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[fs-irods#18] when closing data objects at process exit, close iRODSFS handles first#611

Draft
d-w-moore wants to merge 1 commit into
irods:mainirods/python-irodsclient:mainfrom
d-w-moore:irodsfs-18.md-w-moore/python-irodsclient:irodsfs-18.mCopy head branch name to clipboard
Draft

[fs-irods#18] when closing data objects at process exit, close iRODSFS handles first#611
d-w-moore wants to merge 1 commit into
irods:mainirods/python-irodsclient:mainfrom
d-w-moore:irodsfs-18.md-w-moore/python-irodsclient:irodsfs-18.mCopy head branch name to clipboard

Conversation

@d-w-moore

Copy link
Copy Markdown
Collaborator

No description provided.

@trel

trel commented Aug 23, 2024

Copy link
Copy Markdown
Member

why do we need this here? importing a project that depends on this one?

@d-w-moore

Copy link
Copy Markdown
Collaborator Author

why do we need this here? importing a project that depends on this one?

If the iRODSFS class is successfully loaded into the interpreter, it means there can be file handles needing to be closed.

@trel

trel commented Aug 23, 2024

Copy link
Copy Markdown
Member

Shouldn't... they worry about that? Can discush.

@d-w-moore

Copy link
Copy Markdown
Collaborator Author

Shouldn't... they worry about that? Can discush.

Yes they should, in an ideal design.

@d-w-moore d-w-moore marked this pull request as draft August 23, 2024 12:42
@d-w-moore

d-w-moore commented Aug 23, 2024

Copy link
Copy Markdown
Collaborator Author

I wouldn't normally refer to higher abstractions within a lower-level code base, but this one might be special. iRODSFS library has very good potential for making high level file objects with all the nice abstractions one is used to seeing in I/O libraries - such as a choice of Unicode encodings, for example.

fs = iRODSFS(session)
file_object =  fs.open('/path/to/wide_char_output', 'w', encoding='utf-16')

I admit this code change to be introducing an ugly reverse dependency, inserted in a surprising way, so I've made it a draft PR now. In its defense though, there is a built-in fallback in case the other library isn't there.

Potentially there's a way to make this into a more generic interface, the way we did for progress bars. I'll have to give that some thought.

@d-w-moore

d-w-moore commented Aug 23, 2024

Copy link
Copy Markdown
Collaborator Author

I haven't heard from @hechth in a while, so I'm wondering if we should not just make our own fork for internal use - or, just bring it in as part of PRC. Maybe fs-irods would then get imported as irods.fs

@trel

trel commented Aug 23, 2024

Copy link
Copy Markdown
Member

interesting. his project is MIT Licensed... so we'd need to work that out...

@korydraughn

Copy link
Copy Markdown
Contributor

@d-w-moore Let's discush this.

@d-w-moore

Copy link
Copy Markdown
Collaborator Author

So any use of or reference to it is protected in some way?... hmm. But he did mention at some point it would be desirable to turn it over to us.

@d-w-moore

Copy link
Copy Markdown
Collaborator Author

@d-w-moore Let's discush this.

Certainly. We've got time too, there is no rush on this.

@d-w-moore

d-w-moore commented Aug 25, 2024

Copy link
Copy Markdown
Collaborator Author

interesting. his project is MIT Licensed... so we'd need to work that out...

Ok, we wouldn't have to make it part of the PRC - probably not a good idea anyway. Theoretically we wouldn't have to adopt the MIT license if we treated fs_irods.iRODSFS as an module or plugin to be imported by us, and push all the iRODSFS-related cleanup code to be part of an optional finalize( ) method inside of that module.

@trel

trel commented Aug 25, 2024

Copy link
Copy Markdown
Member

correct, separate modules can be licensed independently (as they are right now).

right, all cleanup should/must happen on the importer's side, not ours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.