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

Switch the usage of UseIn #76

Closed
oliverpool opened this issue Dec 5, 2016 · 3 comments
Closed

Switch the usage of UseIn #76

oliverpool opened this issue Dec 5, 2016 · 3 comments

Comments

@oliverpool
Copy link
Contributor

Hi,

I find strange that store.UseIn(composer) only acts on the composer (and not on the store).

It might be possible to create a UseAllExtensions method on the composer using some tests to know what the store implements (see https://stackoverflow.com/a/20315155/3207406)

For instance:

func (composer *tusd.StoreComposer) UseAllExtensions(store interface{}) {
  var ok bool
  if _, ok = store.(DataStore);ok {
    composer.UseCore(store)
  }
  if _, ok = store.(LockerDataStore);ok {
    composer.UseLocker(store)
  }
  // ...
}
@oliverpool
Copy link
Contributor Author

The main goal is to simplify the example of the Readme:

    // A storage backend for tusd may consist of multiple different parts which
    // handle upload creation, locking, termination and so on. The composer is a
    // place where all those seperated pieces are joined together. In this example
    // we only use the file store but you may plug in multiple.
    composer := tusd.NewStoreComposer()
    compoer.UseAllExtensions(store) // instead of store.UseIn(composer)

If you think it might be a good idea, I can make a PR with this.

@Acconut
Copy link
Member

Acconut commented Dec 5, 2016

When I first designed the StoreComposer API, I used a similar approach as you are proposing here but it had its flaws. First of all, it does not properly work if you have a struct embedding a DataStore as it will hide all additional interfaces which are implemented by the embedded struct. Here is an example to help you understand this issue: https://play.golang.org/p/Eih7d2FvqH. This behavior is un-intuitive and I was very surprised when I first encountered it. Therefore I would prefer to save people the time of catching such a bug.

Secondly, I like my code explicit. It is way more descriptive to tell exactly which extension should be used in a given composer. It makes reading, understanding and debugging easier and that's even worth the additional lines which have to be written.

Finally, a StoreComposer will be created using exactly this method (as you described) when you pass a DataStore instead of an StoreComposer in the configuration (see https://github.com/tus/tusd/blob/master/config.go#L65-L68 and https://github.com/tus/tusd/blob/master/composer.go#L29-L50). This code is not there because I like it but simply to keep it backwards-compatible because the StoreComposer was added later as I discovered weird behavior with interfaces as I described in the first paragraph.

All in all I am not an eager fan of this idea but I am looking forward to your answer.

@oliverpool
Copy link
Contributor Author

I saw your example and didn't know about this behavior: thanks for pointing it out!

Maybe you could add a link to your example (or this issue) in the source so that it appears in the godoc

    // The usage of this field is deprecated and should be avoided in favor of
    // StoreComposer (see https://github.com/tus/tusd/issues/76).

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

No branches or pull requests

2 participants