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

Allows to disable making parent collection while writing stream #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

the-plate
Copy link

Sometimes it is useful and also not necessary to call the createParentCollection(). Hence, a bool param is introduced disabling the function call.

@chripo
Copy link
Member

chripo commented Oct 13, 2023

Thanks for your feature.

We could move this to a global option/setter to make this work for all operations.
Or a refactoring into a decorator that handles the recursive collection creation.

Discussions are welcome.

@the-plate
Copy link
Author

What do you mean by 'all operations'? If I see it right, there are only 3 occurrence of the createParentCollection().
Hence, by global option is meant an additional Client param e.g. skipcreateParents, which would be than checked before each createParentCollection() call... ?

@chripo
Copy link
Member

chripo commented Oct 14, 2023

yes. this PR captures just one call to createPatentCollection().
as a Client setter createPatentCollection(true|false) would make the feature available for all present and future write operations.
the check would be done inside createPatentCollection(path).

@the-plate the-plate force-pushed the improvement/writeStream-without-parent branch from 035a1a6 to 4852177 Compare October 17, 2023 10:32
@the-plate
Copy link
Author

the-plate commented Oct 17, 2023

Hence, I've adjusted the changes based on your thoughts and suggestion. Please have a look at it.

@ueffel
Copy link
Collaborator

ueffel commented Oct 18, 2023

Just my opinions:

  • I don't like the variadic bool argument. I think it's not intuitive why the argument is variadic, as a user I would ask myself "why can I supply an arbitrary number of bool parameters?" I understand that you chose that way to keep compatibility, but I would rather have a new method WriteWithCreateParentCollection(string, []byte, os.FileMode, bool) with a single bool. The current Write(string, []byte, os.FileMode) can then just call WriteWithCreateParentCollection(string, []byte, os.FileMode, true) to keep compatibility. (The name of the new method is debatable ;))
  • I support the decision to keep the choice of creating parent collections on a per-operation basis rather than a "global" bool setting in Client. It's more flexible this way, I think.

@the-plate
Copy link
Author

@ueffel thanks for the insight!
First, yes you're right about the placement of variadic arg. It is used to ensure compatibility.
Second, personally to parametrize the createParentCollection(bool) is not the prefered way for me either, I would keep it like it is now. Instead, I would parametrize the Write or any other method calling the createParentCollection() fcn to be or not to be called via a condition statement, as you pointed out.
Third and to sum up - I agree with your suggestion, I see it quite the same in respect to the point 2. Therefore, I'll modify it based on the suggestion from point 1.

@chripo
Copy link
Member

chripo commented Oct 19, 2023

I would not be in favor of more method arguments for feature toggling.
For the shake of consistency, I'd prefer a global flag, it's KISS.
Or some kind of Client decorator witch takes care of parent collections.
The basic intent of the project is to be consistent with a file API.

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.

3 participants