gowebdav icon indicating copy to clipboard operation
gowebdav copied to clipboard

Allows to disable making parent collection while writing stream

Open the-plate opened this issue 2 years ago • 7 comments

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

the-plate avatar Oct 13 '23 08:10 the-plate

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.

chripo avatar Oct 13 '23 19:10 chripo

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... ?

the-plate avatar Oct 13 '23 21:10 the-plate

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).

chripo avatar Oct 14 '23 08:10 chripo

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

the-plate avatar Oct 17 '23 10:10 the-plate

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.

ueffel avatar Oct 18 '23 21:10 ueffel

@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.

the-plate avatar Oct 19 '23 08:10 the-plate

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.

chripo avatar Oct 19 '23 09:10 chripo