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

Use of createIO in NWBRecording::openFile #52

Open
oruebel opened this issue Aug 2, 2024 · 0 comments
Open

Use of createIO in NWBRecording::openFile #52

oruebel opened this issue Aug 2, 2024 · 0 comments
Assignees
Labels
category: proposal proposed enhancements or new features priority: low alternative solution already working and/or relevant to only specific user(s)

Comments

@oruebel
Copy link
Contributor

oruebel commented Aug 2, 2024

const std::string& IOType)

is specified as string, but the permissible values are not clear. If we want to use this pattern, we should define an enum in Types.hpp to explicitly define the valid values. The same comment then also applies to:

inline std::unique_ptr<BaseIO> createIO(const std::string& type,

Also, IOType should be renamed to ioType for consistency.

On a related note, I'm wondering whether it would be simpler and more explicit to have the user create the IO object and pass it in as a BaseIO pointer instead of using createIO to internally create the IO object for the user:

createIO(IOType, filename));

The main advantage of creating the IO object on behalf of the user seems to be that it ensures that we have managed unique pointer that will be cleaned up:

https://github.com/NeurodataWithoutBorders/aqnwb/blob/00aa1caff04ce9938b4d07c937859cdc34004539/src/Utils.hpp#L65

However, this pattern makes it hard to have custom flags on IO backends. The user could still use the createIO convenience function if they choose to for the common use-case.

I.e., I think NWBRecording::openFile should just take a BaseIO pointer as input instead of having the IOType string.

@oruebel oruebel added category: proposal proposed enhancements or new features priority: low alternative solution already working and/or relevant to only specific user(s) labels Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: proposal proposed enhancements or new features priority: low alternative solution already working and/or relevant to only specific user(s)
Projects
None yet
Development

No branches or pull requests

2 participants