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

Improved time-series support for financial reports. #753

Merged
merged 13 commits into from
Mar 20, 2024

Conversation

nocodehummel
Copy link
Contributor

@nocodehummel nocodehummel commented Mar 17, 2024

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
  • Other Bugfix
  • [X ] Docs
  • Chore/other

Comments/notes

Awaiting feedback on BREAKING CHANGE and:

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

Copy link

codecov bot commented Mar 17, 2024

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nocodehummel nocodehummel marked this pull request as draft March 17, 2024 18:19
@nocodehummel nocodehummel marked this pull request as ready for review March 18, 2024 08:06
@nocodehummel nocodehummel marked this pull request as draft March 19, 2024 10:39
@nocodehummel
Copy link
Contributor Author

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

@nocodehummel nocodehummel marked this pull request as ready for review March 19, 2024 10:57
@nocodehummel nocodehummel changed the title Implement missing data points in financial statements Improved time-series support for financial reports. Mar 19, 2024
@gadicc
Copy link
Owner

gadicc commented Mar 19, 2024

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:

  1. 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 😅 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.

  2. 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.

  3. 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
Copy link
Contributor

Awesome work, LGTM! 🎉

@nocodehummel
Copy link
Contributor Author

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

@nocodehummel
Copy link
Contributor Author

nocodehummel commented Mar 19, 2024

@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 nocodehummel marked this pull request as draft March 19, 2024 18:15
@nocodehummel
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@nocodehummel nocodehummel marked this pull request as ready for review March 20, 2024 08:31
@gadicc
Copy link
Owner

gadicc commented Mar 20, 2024

@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 gadicc merged commit 188860a into gadicc:devel Mar 20, 2024
3 checks passed
gadicc pushed a commit that referenced this pull request Mar 20, 2024
# [2.11.0](v2.10.0...v2.11.0) (2024-03-20)

### Bug Fixes

* **pkg:** commit yarn.lock with @types/har-format ([b1270b5](b1270b5))

### Features

* **fundamentalsTimeSeries:** improved financial reports ([#753](#753)) ([188860a](188860a))
@gadicc
Copy link
Owner

gadicc commented Mar 20, 2024

🎉 This PR is included in version 2.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nocodehummel nocodehummel deleted the 752-timeseries-missing-data-points branch March 20, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fundamental timeseries is missing data points
3 participants