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

fix(yahoo): YF_QUERY_HOST in URLs (user configurable, default query2) #109

Merged
merged 13 commits into from
Mar 6, 2022

Conversation

gadicc
Copy link
Owner

@gadicc gadicc commented Apr 3, 2021

Closes #95.
Closes #426.

Maybe a WIP for #95 or perhaps full first part of it that we could merge now.

  • In modules: use URLs like https://${YF_QUERY_HOST}/blah
  • In modules: avoid template literals to add ${query}
  • In yahooFinanceFetch: do variable substitution, randomly picking between query1 / query2
  • In fetchDevel: standardize to query2 for cache
  • Substitute query2 in all existing tests/http files (query1 was much less common)

@gadicc
Copy link
Owner Author

gadicc commented Apr 3, 2021

May also be better to just have a default single YF_QUERY_HOST and let the user change it as desired, rather than picking randomly on every request, which could make it harder to find errors if one host starts acting up.

@codecov
Copy link

codecov bot commented Apr 3, 2021

Codecov Report

Merging #109 (b4da723) into devel (7bc89e0) will not change coverage.
The diff coverage is 100.00%.

❗ Current head b4da723 differs from pull request most recent head 6e5c579. Consider uploading reports for the commit 6e5c579 to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##             devel      #109   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          461       468    +7     
  Branches       146       150    +4     
=========================================
+ Hits           461       468    +7     
Impacted Files Coverage Δ
src/modules/chart.ts 100.00% <ø> (ø)
src/modules/historical.ts 100.00% <ø> (ø)
src/modules/insights.ts 100.00% <ø> (ø)
src/modules/options.ts 100.00% <ø> (ø)
src/modules/quote.ts 100.00% <ø> (ø)
src/modules/quoteSummary.ts 100.00% <ø> (ø)
src/modules/recommendationsBySymbol.ts 100.00% <ø> (ø)
src/modules/search.ts 100.00% <ø> (ø)
src/modules/trendingSymbols.ts 100.00% <ø> (ø)
src/lib/options.ts 100.00% <100.00%> (ø)
... and 1 more

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 7bc89e0...6e5c579. Read the comment docs.

@gadicc gadicc changed the title chore(yahoo): Use randomized YF_QUERY_HOST in URLs (#95) chore(yahoo): Specify / randomize YF_QUERY_HOST in URLs (#95) Mar 6, 2022
@gadicc
Copy link
Owner Author

gadicc commented Mar 6, 2022

  • Rebase on latest devel
  • Recheck all modules
  • Recheck all tests
  • Allow to specify a specific host, default to query2, or allow (maybe?) allow random.

grep -l query1 tests/http/*.json \
  | xargs sed -i 's/query1.finance.yahoo.com/query2.finance.yahoo.com/g'
@gadicc gadicc changed the title chore(yahoo): Specify / randomize YF_QUERY_HOST in URLs (#95) fix(yahoo): Specify / randomize YF_QUERY_HOST in URLs Mar 6, 2022
@gadicc gadicc merged commit 716c0f1 into devel Mar 6, 2022
gadicc pushed a commit that referenced this pull request Mar 6, 2022
# [2.3.0](v2.2.0...v2.3.0) (2022-03-06)

### Bug Fixes

* **insights:** fix schema for more recent yahoo results ([c71a400](c71a400))
* **validation:** Date, allow ISODate without ms precision too ([82ba1cb](82ba1cb))

### Features

* **yahoo:** Configurable YF_QUERY_HOST in URLs ([#109](#109)) ([716c0f1](716c0f1))
@gadicc
Copy link
Owner Author

gadicc commented Mar 6, 2022

🎉 This PR is included in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gadicc gadicc added the released label Mar 6, 2022
@gadicc gadicc deleted the query2 branch March 6, 2022 16:05
@gadicc
Copy link
Owner Author

gadicc commented Mar 6, 2022

Just a note here, in the end, I didn't randomize the host, because:

  1. That will be much harder to debug
  2. It was pointed out that query1 is HTTP/1.0 and query2 is HTTP/2.0
  3. So we default to query2 but allow the user to change it.

Ref #426.

@gadicc gadicc changed the title fix(yahoo): Specify / randomize YF_QUERY_HOST in URLs fix(yahoo): YF_QUERY_HOST in URLs (user configurable, default query2) Mar 6, 2022
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.

Allow to switch root url to query2.finance.yahoo.com Use both query1 and query2.
1 participant