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

Hotfix/4526 #4682

Merged
merged 30 commits into from
Apr 12, 2023
Merged

Hotfix/4526 #4682

merged 30 commits into from
Apr 12, 2023

Conversation

colin99d
Copy link
Contributor

@colin99d colin99d commented Apr 4, 2023

Description

Fixes #4526

This hits the nasdaq API directly, instead of web scraping market watch. So this is better in every way.

How has this been tested?

  • Please describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.
  • Make sure affected commands still run in terminal
  • Ensure the SDK still works
  • Check any related reports

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.

@reviewpad reviewpad bot added the feat XS Extra small feature label Apr 4, 2023
@reviewpad reviewpad bot added feat S Small T-Shirt size Feature and removed feat XS Extra small feature labels Apr 4, 2023
@reviewpad reviewpad bot added feat XL Extra Large feature and removed feat S Small T-Shirt size Feature labels Apr 4, 2023
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

❗ No coverage uploaded for pull request base (develop@9435fd7). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #4682   +/-   ##
==========================================
  Coverage           ?   57.51%           
==========================================
  Files              ?      588           
  Lines              ?    53792           
  Branches           ?        0           
==========================================
  Hits               ?    30941           
  Misses             ?    22851           
  Partials           ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@deeleeramone
Copy link
Contributor

deeleeramone commented Apr 5, 2023

2023 Apr 05, 13:06 (🦋) /stocks/fa/ $ sec --source FinancialModelingPrep

No filings found for ticker AAPL, consider increasing the value for --pages. Showing recent filings instead.

This source does not hit the correct endpoint for the function. The correct endpoint is here: https://site.financialmodelingprep.com/developer/docs/#Financial-Statements-List

The i18n file needs to be updated as well.

Prints SEC filings of the company. The following fields are expected: Filing Date, Document Date, Type, Category, Amended, and Link. [Source: Market Watch and FinancialModelingPrep]

@colin99d
Copy link
Contributor Author

colin99d commented Apr 6, 2023

Im going to add the FMP endpoint, but it has some limitations, and it doesn't seem useful to me now that we are pulling directly from Nasdaq.

@colin99d
Copy link
Contributor Author

colin99d commented Apr 6, 2023

Actually, per Jeroen it should just be deleted.

@colin99d colin99d added the enhancement Enhancement label Apr 6, 2023
@deeleeramone
Copy link
Contributor

@colin99d

(🦋) /stocks/fa/ $ sec

Year will be ignored if form_group is not specified
No data found for AAPL

@deeleeramone
Copy link
Contributor

deeleeramone commented Apr 8, 2023

I suggest upping the default value for --limit to be 100 instead of 20, thereby ensuring all results from the year and form type are returned.

When an invalid form group is selected, the response is not uniform. quarter returns nothing, but a random string of characters returns the default output. Ideally, an error message is printed indicating that the choice is not valid, what the valid choices are, and what results are being returned instead of the error.

Screenshot 2023-04-07 at 8 25 47 PM

There are a few items that need to be updated still:

  • The help dialogue in the controller needs to be updated still.

Screenshot 2023-04-07 at 8 11 46 PM

  • The description of the attribute within the StocksFundamentalAnalysis class:

Screenshot 2023-04-07 at 8 14 07 PM

The docstrings are pointing at a file that shouldn't exist anymore because the sources have both been replaced by a new one.

Screenshot 2023-04-07 at 8 16 13 PM

One item which may fall out of scope for this PR, because it is also all items in the module, is that with the new tables, the view aspect of the SDK becomes relevant and a reason to include the _chart version of each function. _chart for all functions that returns a DataFrame can be a choice for using the interactive tables.

Copy link
Collaborator

@jmaslek jmaslek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@deeleeramone
Copy link
Contributor

I have a question, why is there a new file called, "marketwatch_model.py", which only contains one function - which is sourced from Nasdaq? There is already market_watch_model, this seems oddly confusing.

Screenshot 2023-04-11 at 2 26 05 PM

@colin99d colin99d added this pull request to the merge queue Apr 12, 2023
Merged via the queue into develop with commit 2e79308 Apr 12, 2023
jmaslek added a commit that referenced this pull request Apr 13, 2023
* Added fixes

* Merged

* Merged

* Added some handling of errors

* Adds some read dbs

* Removed useless commands

* Added fixes

* Added fixes

* Added fix

* Added fix

* Added fix

* Added fix

* Added fix

* Added fix

* Fixed tests

* Handle lowercase

* Added fxes

* small ticker thing

---------

Co-authored-by: James Maslek <jmaslek11@gmail.com>
@piiq piiq deleted the hotfix/4526 branch April 24, 2023 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement feat XL Extra Large feature
Projects
None yet
3 participants