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

remove flumeview-query/links fork #4

Closed
dominictarr opened this issue Oct 19, 2017 · 3 comments
Closed

remove flumeview-query/links fork #4

dominictarr opened this issue Oct 19, 2017 · 3 comments

Comments

@dominictarr
Copy link
Contributor

I am very disapointed that the contents of the lib folder is copy pasted from flumeview-query.
Why didn't you make a PR? copying my code might save you time one day, but it means you are now maintaining my code, and since you didn't even post an issue, I didn't even know where doing this until I read the code because of the forks discussion. You are just creating more work for yourself and making my work less effective!

@mmckegg
Copy link
Contributor

mmckegg commented Oct 19, 2017

It was always my intention to do these changes as a PR, but I couldn't figure out an elegant way to merge this into the existing flumeview-query module. Besides the code is simple enough. It is mostly just routing options into place. Could be simplified right down if it was just merged into index.js

I'm really not interested in bikeshedding an API so I don't want to discuss how to implement this as options in flumeview-query, but if you really want to remove this duplicate code, then go ahead and make a PR!

Here's what would need to be added to flumeview-query

@dominictarr
Copy link
Contributor Author

At the very least, you should post an issue to let me know you are doing this.
If my api doesn't meet your needs, that is something I need to know!

why does it return the original item?

@mmckegg mmckegg changed the title this should have been a PR remove flumeview-query/links fork Oct 25, 2017
@mmckegg
Copy link
Contributor

mmckegg commented Oct 25, 2017

@dominictarr

why does it return the original item?

This module is used to retrieve related messages. If it didn't return the original item, the client code would need to perform a second lookup to get the actual item. Why not just return it as part of the index?

It could be an option like {values: true} when calling links.read


How do you think we should implement the unbox functionality? Are you happy with passing in an option to the constructor?

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

No branches or pull requests

3 participants