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

Polygon maxretry showpath #485

Merged
merged 2 commits into from
Jul 6, 2024
Merged

Polygon maxretry showpath #485

merged 2 commits into from
Jul 6, 2024

Conversation

Canx
Copy link
Collaborator

@Canx Canx commented Jul 6, 2024

Better polygon message, showing REST API call path to better know which plan is needed.

@Canx Canx requested a review from grzesir as a code owner July 6, 2024 08:05
Copy link
Contributor

korbit-ai bot commented Jul 6, 2024

My review is in progress 📖 - I will have feedback for you in a few minutes!

Copy link
Contributor

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

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

I have reviewed your code and found 2 potential issues. To discuss my individual comments that I have added, tag me in replies using @korbit-ai.


Please react with a 👍 to my comments that you find helpful and a 👎 to those you find unhelpful - this will help me learn and improve as we collaborate.

Comment on lines +558 to 565
message = (
"Polygon rate limit reached.\n\n"
f"REST API call affected: {url}\n\n"
f"Sleeping for {PolygonClient.WAIT_SECONDS_RETRY} seconds seconds before trying again.\n\n"
"If you want to avoid this, consider a paid subscription with Polygon at https://polygon.io/?utm_source=affiliate&utm_campaign=lumi10\n"
"Please use the full link to give us credit for the sale, it helps support this project.\n"
"You can use the coupon code 'LUMI10' for 10% off."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

category Logging Severity Major

The log message in the MaxRetryError exception handling block is too verbose and contains sensitive information. Please simplify the log message and remove the coupon code from the log. Consider sending the coupon code to the user through a more secure channel.

colored_message = colored(
f"Polygon rate limit reached. Sleeping for {PolygonClient.WAIT_SECONDS_RETRY} seconds before trying again. If you want to avoid this, consider a paid subscription with Polygon at https://polygon.io/?utm_source=affiliate&utm_campaign=lumi10 Please use the full link to give us credit for the sale, it helps support this project. You can use the coupon code 'LUMI10' for 10% off.",
"red",
url = urlunparse(urlparse(kwargs['path'])._replace(query=""))
Copy link
Contributor

Choose a reason for hiding this comment

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

category Naming

The variable name 'url' in the MaxRetryError exception block is not descriptive enough to indicate its purpose. Consider renaming it to 'sanitized_url_path' to clarify that it represents the URL path without query parameters.

@Canx Canx merged commit eeb25ca into dev Jul 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants