Fix for demo_forex.py and update to README.md

jdarias commented 1 year ago

demo_forex.py is broken because requests is blocked by cloudflare. Information on the second line is not displayed, as the request for https://tr.investing.com is a content blocked page that does not have anything the script is looking for. This is a fix that uses cloudscraper instead of requests to bypass the restriction. Updates to Readme: Reflect the dependency changes on demo_forex.py. An omission is corrected regarding demo_tiny_dashboard.py and the quote character length limits that this demo now supports.

jdarias commented 1 year ago

Hi Carlos I created a new pull request and I'm closing this one. I will do the amend to my script later. Have a good night!

cgomesu commented 1 year ago

you shouldn't open a new PR to add the changes per a reviewer's feedback--and that's frowned upon because it makes it harder for the maintainers to track the review process (e.g., now my original review will be attached to #65 but the changes will be in #66, which is very confusing). if a maintainer asks you to amend a PR, then go back to your branch (demo_forex_fix), make the modifications there, commit the changes, and push them to the remote (jdarias:demo_forex_fix), which will make the changes show up in the PR under review automatically.

I'll go ahead and reopen this PR that contains my original review and close the new one. all that you need to do is to add the changes from jdarias:demo_forex_update to the jdarias:demo_forex branch and they will show up below. I'll then continue the review process.

jdarias commented 1 year ago

Carlos do you mean I just do the changes and do git commit --amend on my branch? My bad about the extra pull and branch, I'm absolutely new to the collaborative side of git/github

On Thu, Jun 8, 2023, 08:54 Carlos Gomes @.***> wrote:

you shouldn't open a new PR to add the changes per a reviewer's feedback--and that's frowned upon because it makes it harder for the maintainers to track the review process (e.g., now my original review will be attached to #65 https://github.com/the-raspberry-pi-guy/lcd/pull/65 but the changes will be in #66 https://github.com/the-raspberry-pi-guy/lcd/pull/66, which is very confusing). if a maintainer asks you to amend a PR, then go back to your branch (demo_forex_fix), make the modifications there, commit the changes, and push them to the remote (jdarias:demo_forex_fix), which will make the changes show up in the PR under review automatically.

I'll go ahead and reopen this PR that contains my original review and close the new one. all that you need to do is to add the changes from jdarias:demo_forex_update to the jdarias:demo_forex branch and they will show up below. I'll then continue the review process.

— Reply to this email directly, view it on GitHub https://github.com/the-raspberry-pi-guy/lcd/pull/65#issuecomment-1582620494, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJBLGXVWOAV6EXTNG7GJRTXKHKQLANCNFSM6AAAAAAYZO5EUQ . You are receiving this because you modified the open/close state.Message ID: @.***>

cgomesu commented 1 year ago

Carlos do you mean I just do the changes and do git commit --amend on my branch?

no. we're now under a review process. I made a few comments about the original fix--namely, I disagreed with the proposed solution and mentioned an alternative. you need to reply my comments first. your reply will tell which course of action we should take here. for example, you might have already tested the solution I mentioned and noticed that it doesn't work. however, if you agree with my comments and tested the alternative URL, then what you need to do is edit the files in this PR to reflect the suggested changes. to edit such files, you just need to do the following:

on you local machine, go to the dir that contains the clone of this project and switch to the branch demo_forex_fix branch via git checkout demo_forex_fix (because that is the from branch in this PR). now, just edit the files per my review and once you're done, add them (git add) and commit the changes (git commit) writing an informative message about those changes. finally, simply push the changes to your jdarias:demo_forex_fix remote and the changes will show up here automatically because a PR is just a link between two branches. it's that simple.

My bad about the extra pull and branch, I'm absolutely new to the collaborative side of git/github

no worries. I've "been there, done that", too.

jdarias commented 1 year ago

Carlos, I attempted using requests to the major indices URL you suggested, unfortunately requests gets the same blocked response.

Is it okay to keep on using cloudscrapper instead of requests and send the request to the major-indices URL?

cgomesu commented 1 year ago

Carlos, I attempted using requests to the major indices URL you suggested, unfortunately requests gets the same blocked response.

please post here the entire request code and response. I can retrieve data from https://tr.investing.com/indices/major-indices just fine via curl.

jdarias commented 1 year ago

It's the same response I put on previous communication but here you go:

>>> import requests
>>> fakeheaders= {  'User-Agent': 'Chrome/91.0.4472.124 Safari/537.36 Edg/91.0.864.59' }
>>> hr=requests.get(url="https://tr.investing.com/indices/major-indices", headers=fakeheaders)
>>> print(hr.content)
b'<!DOCTYPE html>\n<!--[if lt IE 7]> <html class="no-js ie6 oldie" lang="en-US"> <![endif]-->\n<!--[if IE 7]>    <html class="no-js ie7 oldie" lang="en-US"> <![endif]-->\n<!--[if IE 8]>    <html class="no-js ie8 oldie" lang="en-US"> <![endif]-->\n<!--[if gt IE 8]><!--> <html class="no-js" lang="en-US"> <!--<![endif]-->\n<head>\n<title>Attention Required! | Cloudflare</title>\n<meta charset="UTF-8" />\n<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />\n<meta http-equiv="X-UA-Compatible" content="IE=Edge" />\n<meta name="robots" content="noindex, nofollow" />\n<meta name="viewport" content="width=device-width,initial-scale=1" />\n<link rel="stylesheet" id="cf_styles-css" href="/cdn-cgi/styles/cf.errors.css" />\n<!--[if lt IE 9]><link rel="stylesheet" id=\'cf_styles-ie-css\' href="/cdn-cgi/styles/cf.errors.ie.css" /><![endif]-->\n<style>body{margin:0;padding:0}</style>\n\n\n<!--[if gte IE 10]><!-->\n<script>\n  if (!navigator.cookieEnabled) {\n    window.addEventListener(\'DOMContentLoaded\', function () {\n      var cookieEl = document.getElementById(\'cookie-alert\');\n      cookieEl.style.display = \'block\';\n    })\n  }\n</script>\n<!--<![endif]-->\n\n\n</head>\n<body>\n  <div id="cf-wrapper">\n    <div class="cf-alert cf-alert-error cf-cookie-error" id="cookie-alert" data-translate="enable_cookies">Please enable cookies.</div>\n    <div id="cf-error-details" class="cf-error-details-wrapper">\n      <div class="cf-wrapper cf-header cf-error-overview">\n        <h1 data-translate="block_headline">Sorry, you have been blocked</h1>\n        <h2 class="cf-subheadline"><span data-translate="unable_to_access">You are unable to access</span> investing.com</h2>\n      </div><!-- /.header -->\n\n      <div class="cf-section cf-highlight">\n        <div class="cf-wrapper">\n          <div class="cf-screenshot-container cf-screenshot-full">\n            \n              <span class="cf-no-screenshot error"></span>\n            \n          </div>\n        </div>\n      </div><!-- /.captcha-container -->\n\n      <div class="cf-section cf-wrapper">\n        <div class="cf-columns two">\n          <div class="cf-column">\n            <h2 data-translate="blocked_why_headline">Why have I been blocked?</h2>\n\n            <p data-translate="blocked_why_detail">This website is using a security service to protect itself from online attacks. The action you just performed triggered the security solution. There are several actions that could trigger this block including submitting a certain word or phrase, a SQL command or malformed data.</p>\n          </div>\n\n          <div class="cf-column">\n            <h2 data-translate="blocked_resolve_headline">What can I do to resolve this?</h2>\n\n            <p data-translate="blocked_resolve_detail">You can email the site owner to let them know you were blocked. Please include what you were doing when this page came up and the Cloudflare Ray ID found at the bottom of this page.</p>\n          </div>\n        </div>\n      </div><!-- /.section -->\n\n      <div class="cf-error-footer cf-wrapper w-240 lg:w-full py-10 sm:py-4 sm:px-8 mx-auto text-center sm:text-left border-solid border-0 border-t border-gray-300">\n  <p class="text-13">\n    <span class="cf-footer-item sm:block sm:mb-1">Cloudflare Ray ID: <strong class="font-semibold">7d858902da0e12e5</strong></span>\n    <span class="cf-footer-separator sm:hidden">&bull;</span>\n    <span id="cf-footer-item-ip" class="cf-footer-item hidden sm:block sm:mb-1">\n      Your IP:\n      <button type="button" id="cf-footer-ip-reveal" class="cf-footer-ip-reveal-btn">Click to reveal</button>\n      <span class="hidden" id="cf-footer-ip">181.62.52.135</span>\n      <span class="cf-footer-separator sm:hidden">&bull;</span>\n    </span>\n    <span class="cf-footer-item sm:block sm:mb-1"><span>Performance &amp; security by</span> <a rel="noopener noreferrer" href="https://www.cloudflare.com/5xx-error-landing" id="brand_link" target="_blank">Cloudflare</a></span>\n    \n  </p>\n  <script>(function(){function d(){var b=a.getElementById("cf-footer-item-ip"),c=a.getElementById("cf-footer-ip-reveal");b&&"classList"in b&&(b.classList.remove("hidden"),c.addEventListener("click",function(){c.classList.add("hidden");a.getElementById("cf-footer-ip").classList.remove("hidden")}))}var a=document;document.addEventListener&&a.addEventListener("DOMContentLoaded",d)})();</script>\n</div><!-- /.error-footer -->\n\n\n    </div><!-- /#cf-error-details -->\n  </div><!-- /#cf-wrapper -->\n\n  <script>\n  window._cf_translation = {};\n  \n  \n</script>\n\n</body>\n</html>\n'
>>>
>>> print(hr.headers)
{'Date': 'Fri, 16 Jun 2023 19:42:18 GMT', 'Content-Type': 'text/html; charset=UTF-8', 'Transfer-Encoding': 'chunked', 'Connection': 'keep-alive', 'Cache-Control': 'max-age=15', 'Expires': 'Fri, 16 Jun 2023 19:42:33 GMT', 'X-Frame-Options': 'SAMEORIGIN', 'Set-Cookie': '__cf_bm=2rH2FPbkO4jWgn.MRc2d4MyXvet0rfPRUl_Pnket0z8-1686944538-0-AZs2pQfSxghKMfuUn7xm9QH+hcCbBPDTXCJN2XgX7GD2h722njU/QvBOPOtArmhD0nGK5LlmFKrWhE/AQCC1cGM=; path=/; expires=Fri, 16-Jun-23 20:12:18 GMT; domain=.investing.com; HttpOnly; Secure; SameSite=None', 'Vary': 'Accept-Encoding', 'Server': 'cloudflare', 'CF-RAY': '7d858902da0e12e5-ATL', 'Content-Encoding': 'gzip', 'alt-svc': 'h3=":443"; ma=86400'}

cgomesu commented 1 year ago

that's true... kinda odd that curl is working though.

well, in this case, I agree that changing the package to the one you originally suggested (cloudscraper) is a good compromise. however, we should scrape the major indices page instead, per my last comment.

please make the changes accordingly.

cgomesu commented 11 months ago

@jdarias could you please take a look at this PR? it seems we just need a minor change to it (see my last comment) but I'm not sure if this is still an issue affecting the demo.

the-raspberry-pi-guy / lcd

Fix for demo_forex.py and update to README.md #65