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(modules): add trendingSymbols module #66

Merged
merged 2 commits into from
Feb 18, 2021

Conversation

advaiyalad
Copy link
Contributor

New module trendingSymbols

This PR adds a new module 🎉, trendingSymbols! It provides the symbols that are trending in your region (country in this case, as far as I can tell).

Changes

  • Add new module trendingSymbols
  • Add tests for the module
  • Add docs for the module

Endpoint Params

count - max number of symbols that can be returned.
lang - does nothing???
region - overrides the search country, alternative to passing the query.
corsDomain (not added) - This was provided in #8, but does not actually set the cors domain, so it is not added in the PR (better not to add it than to release a new major version for removing it).

Tests

Tests will not pass until #64 is merged, but tests for the module pass.

Example usage

import yahooFinance from 'yahoo-finance2';
yahooFinance.trendingSymbols('US', { count: 5, lang: 'en-US' }); // default queryOptions

@gadicc What are your thoughts on this? Anything to change?

@gadicc gadicc force-pushed the devel branch 2 times, most recently from 85c25b9 to dd90afa Compare February 18, 2021 06:14
@gadicc gadicc merged commit 9dd1239 into gadicc:devel Feb 18, 2021
@gadicc
Copy link
Owner

gadicc commented Feb 18, 2021

@gadicc What are your thoughts on this? Anything to change?

My thoughts are that you rock 🙌 This is great, @PythonCreator27... thanks for checking all the boxes on this one. Fantastic to get another contributed module! Thanks for all your hard work on this and the other PRs! 🙏

As a side note, I had a little difficulty merging the three different PRs as they all had separate versions of the test files. I think everything is ok now but let me know if anything looks weird on any future pulls/diffs from devel.

@advaiyalad
Copy link
Contributor Author

Sorry for the test files problem! Interestingly enough, some test files that were generated were not from the trendingSymbols module... Most of them were from the quoteSummary module. Any idea why?

@advaiyalad advaiyalad deleted the feat/add-trendingSymbols-module branch February 18, 2021 14:36
@gadicc
Copy link
Owner

gadicc commented Feb 18, 2021

Haha all good. Every few years I feel confident with git and then have to do something relatively simple and realise I know nothing 😅

Oh I did notice something weird about that, because at one point I deleted the files and git thought I had renamed them. Thought I'd done something weird but that makes sense that they had similar results to quoteSummary.

The only thing I can think of is that maybe you copied and pasted something and ran the test before adjusting for that module. I use wallabyjs that re-runs all relevant tests on every keystroke, so I've had to be careful with that. Although I'm pretty sure I added some logic to double check the URL when loading from the cache.

I'm off for the day but I'd be grateful if you could take a quick look at the cache that's currently comitted to devel and see if it looks right now and confirm. I'll check back in tomorrow.

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.

2 participants