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

feat(historical2): add historical2 module #239

Draft
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

advaiyalad
Copy link
Contributor

@advaiyalad advaiyalad commented Jul 13, 2021

Closes #238.

Changes

  • Add a new historical module that does not require CSV, and can also fetch stock splits and dividends data, all in one network request.
  • Fixed a small nitpick while copying over the historical module: Object -> Record<string, unknown>

Type

  • New Module
  • Other New Feature
  • Validation Fix
  • Other Bugfix
  • Docs
  • Chore/other

Comments/notes

Tests and docs to come. @gadicc I think it might be appropriate to deprecate the other historical module, and then rename this one to be historical and delete the old one when we hit v2. What is your opinion on this?

CI failure is not related to anything that this PR changes, it seems to be a fluke?

@gadicc
Copy link
Owner

gadicc commented Jul 15, 2021

Hey @PythonCreator27 , big thanks as always for your contributions here. Sorry I'm lagging a little behind, I should have a chance to look at this properly tomorrow. Thanks for your patience too :)

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #239 (65b38e4) into devel (98141c8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             devel      #239   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines          372       372           
  Branches       104       104           
=========================================
  Hits           372       372           
Impacted Files Coverage Δ
src/modules/historical.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98141c8...65b38e4. Read the comment docs.

@gadicc
Copy link
Owner

gadicc commented Jul 16, 2021

Hey @PythonCreator27, this looks great so far. Great work!

Re CI failure, yes, I've seen this in the past too, async timeout. It's indeed a fluke, probably some timer race condition with the fake timers. As before, I just forced a re-run of the test (with the same commit) and everything passed.

Nice work finding a new source for all the data, and as you say, in 1 network request, great!

What you are proposing sounds reasonable but let's not rush it. Let's merge when ready, keep both modules side by side for a while and see what comes up, and then plan the deprecation carefully.

Maybe (not sure yet), it might be nice to take an option to return a single date-sorted array with all kinds of data, but let's look at that later after this is merged. I wonder if that might also allow us to even switch with the original historical module, without any deprecation, if we plan the options and types carefully. But as I said, let's get this merged first before exploring those options.

Thanks as always for the great work! 🙏

@ZehCoque
Copy link

Hey guys! It was me who first opened this issue and I would like to compliment you once more for the quick response and awesome work! I was wondering if it is possible (even if on an incomplete version) to use historical2 now and, of course, if any help is needed to get everything done. Let me know!

@gadicc
Copy link
Owner

gadicc commented Aug 27, 2021

@PythonCreator27, are you waiting for anything from me on this? I know you still want to add tests and docs.

I'm suggesting now, and please let me know your opinion on this, that we temporarily name the module _historical2 (i.e. prepended with an underscore), to emphasise that it may still be subject to change (without needing to release a new major version). That way we can get it out already and start getting feedback from users, and still be able to make changes.

huned pushed a commit to huned/node-yahoo-finance2 that referenced this pull request Nov 12, 2021
…equest.

1. Apply patch generated from [gadicc#239] that fetches
historical prices, splits, and dividends in a single network request.
Original code by @PythonCreator27.

2. Regenerate schema.

Fixes [gadicc/node-yahoo-finance#238]
@huned huned mentioned this pull request Nov 14, 2021
6 tasks
gadicc pushed a commit that referenced this pull request Dec 21, 2021
# [2.1.0](v2.0.1...v2.1.0) (2021-12-21)

### Bug Fixes

* **chart:** more query tests, intervals, edge cases ([#336](#336)) ([6b95d7e](6b95d7e))
* **package:** have semantic-release recognize version branches ([a89d895](a89d895))
* **quote:** equity: allow underlyingSymbol.  LDN.MI test ([#363](#363)) ([817410b](817410b))

### Features

* **chart:** { return: "array" } (default) + test fix ([#336](#336)) ([1ac66c3](1ac66c3))
* **chart:** initial release as "_chart" ([#239](#239), [#328](#328), [#334](#334)) ([92b90b1](92b90b1))
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.

Stock splits and getting dividends, stock splits, and other historical data in one function call
3 participants