search

xemwebe / yahoo_finance_api

Simple wrapper to yahoo! finance API to retrieve latest quotes and end-of-day quote histories
Apache License 2.0
69 stars 35 forks source link

Collection: Useful traits (Clone, PartialEq, PartialOrd), negative firstTradeDate #2

Closed celaus closed 4 years ago

celaus commented 4 years ago

I needed a few traits for something 👍

celaus commented 4 years ago

I also ran some formatter.

xemwebe commented 4 years ago

Thanks for your contributions! I would like to accept your first three commits and adding WASM compatibility to the crate. However, I am not so happy with your suggestion to replace reqwest for a couple of reasons:

  1. If you use the crate in a larger context where reqwest comes anyway in as a dependency of other dependencies, replacing reqwest doesn't really decrease the size, but has the contrary effect
  2. Without having locked at the code of ureq myself, reviews of this crate seem to be quite mixed, while reqwest is the well-tested quasi-standard.
  3. For some other project, I need support for async/.await, which does not seem to be supported by ureq.

If your main concern with respect to reqwest is openssl dependency, I would suggest to just remove that features (which is enable by default). Also, I would suggest to make the blocking api an optional feature to support both, blocking and async/.await.

celaus commented 4 years ago

yes it was only for the openssl dependency because I wanted the crate to cross compile smoothly 😄 - reqwest actually does provide that feature as well, so I rolled the changes back.

xemwebe commented 4 years ago

That's great, so we only need to resolve the merge conflict, which is caused by the recent upgrade to regwest 0.10 and with that the move from a blocking API to a Futures based API. Therefore, I would remove the blocking parts of your wasm code. Also, I am not happy with using unwrap calls that might panic in case of an error, but return an error instead.

However, since this would require some significant changes to the wasm code, I would like to make sure that it is not broken. Could you provide a small example calling the wasm code from JS?

By the way, since your contribution to this little crate is significant, I would like to add you to the authors list, if you don't mind.

celaus commented 4 years ago

Yes, I'll take a look at some JS and the async transition this weekend 😄 .

Happy to be added to the author's list 👍 .

About the async transition: do you want to keep it fully async? I am asking because users of the sync version (0.2.3 IIRC) won't see some of the fixes in this PR without having to deal with futures. I could maybe add a "sync" feature?

xemwebe commented 4 years ago

Make async the new default, but adding a "sync" or "blocking" feature would also be me favorite way to go forward.