Improved time-series support for financial reports.

nocodehummel commented 3 months ago

Improved the fundamentalsTimeSeries module to retrieve data points for financial reports. This change resolves missing data points in the financial statements. The keys used in the time-series can be maintained with a script.

BREAKING CHANGE:

requires option module.
type string is stripped from the key in the results.

Closes #752.

Changes

introduced option module ('financials', 'balance-sheet', 'cash-flow')
set query type with keys specified per module.
strip option type string from returned data keys.
introduced script to maintain the keys per module.

Type

[ ] New Module
[ ] Other New Feature
[ ] Validation Fix
[X] Other Bugfix
[X ] Docs
[ ] Chore/other

Comments/notes

Awaiting feedback on BREAKING CHANGE and:

should it be possible to call fundamentalsTimeSeries with custom keys?
should deprecated historical financial data queries be removed?

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 97.36842% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 93.56%. Comparing base (7a1d03b) to head (14e568c). Report is 4 commits behind head on devel.

Files	Patch %	Lines
src/modules/fundamentalsTimeSeries.ts	97.36%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## devel #753 +/- ## ========================================== + Coverage 93.12% 93.56% +0.43% ========================================== Files 26 27 +1 Lines 669 730 +61 Branches 226 246 +20 ========================================== + Hits 623 683 +60 - Misses 46 47 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nocodehummel commented 3 months ago

Moved the time-series keys to a JSON file that can be maintained by a script parsing har files.

gadicc commented 3 months ago

Thanks, @nocodehummel, for the phenomenal PR! Great idea to get all the keys from a har file and thanks both for your great implementation and adherence to project styling etc.

This all looks good to me but would love to hear from @Shadaeiou from too. As for your questions:

Agree with the breaking change for module. In theory we should bump a major version but honestly since this has only existed for a week I think we can just push it through :sweat_smile: Potentially we could (temporarily) insert a default value and give a warning to replicate the previous behaviour, but I don't think it's worth the effort under the circumstances.
Re custom keys: probably not, as we wouldn't be able to validate the result / provide correct types. It's a pain to always need to accommodate new keys, but I believe is the right approach and worth the effort. We could however offer ability for custom keys in conjunction with { validateResult: false } if we really wanted.
Re deprecated historical queries, it's a good question. I see some data points still exist, so probably better to leave it, however, it would be great for us to log a warning about the situation. I think that's probably the approach that will be most helpful to people using the library. It's beyond the scope of this PR though, but if you're keen to do it, it will be very welcome :)

Thanks again for your great work here! Super appreciated.

Oh and P.S., re vscode/settings.json. Great to specify the files.eol, thanks, will def make things easier for Windows contributors. Re prettier settings, I'd prefer to leave everything as the prettier default, and add any necessary exceptions to the .prettierignore file.

Shadaeiou commented 3 months ago

Awesome work, LGTM! 🎉

nocodehummel commented 3 months ago

@gadicc and @Shadaeiou; thanks. Looking forward to a new release.

nocodehummel commented 3 months ago

@gadicc for the deprecated historical queries a migration path can be to add three new modules; incomeStatement, balanceSheet and cashflowStatement that map to the fundamentalTimeseries to fetch the data. As far as I understand the package schema validation could be used to verify that the results is matching the previous endpoint. The interface looks similar to me and this could potentially be realized without mapping properties.

At the same time adding a deprecation warning. Would that make sense as a new PR?

nocodehummel commented 3 months ago

Converted back to draft. I noticed on one of my stocks PHIA.AS that the module return 5 quarterly income statement, 3 balance sheets and zero cash-flow. Need to investigate.

nocodehummel commented 3 months ago

Reviewed the results and the PR is good. The number of reports returned is the same as devel without PR. Verified some symbols by comparing with the UI and the number of reports is the same. Added test cases for AAPL to check results contain 5 quarterly reports and 4 annual reports with relevant property.

gadicc commented 3 months ago

@nocodehummel, thanks again for all your amazing work on this! Most especially the extra time on testing and checking results, which will no doubt be greatly appreciated by all our users!

All looks great. I'm going to merge now and this will be in our next release. There'll be an automated message here once it's published. I'll respond separately re the migration path. Thanks so much again, this is great work!

gadicc commented 3 months ago

:tada: This PR is included in version 2.11.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

gadicc / node-yahoo-finance2