-
-
Notifications
You must be signed in to change notification settings - Fork 46.5k
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 get amazon product data erroring due to whitespace in headers #9009
Fix get amazon product data erroring due to whitespace in headers #9009
Conversation
Multiple Pull Request Detected@CaedenPH, we are extremely excited that you want to submit multiple algorithms in this repository but we have a limit on how many pull request a user can keep open at a time. This is to make sure all maintainers and users focus on a limited number of pull requests at a time to maintain the quality of the code. This pull request is being closed as the user already has an open pull request. Please focus on your previous pull request before opening another one. Thank you for your cooperation. User opened pull requests (including this one): #9009, #8966, #8965, #8906 |
Broken bot 😢 |
ouch, is that bot for real though? |
@CaedenPH Last time it stopped you once you exceeded 5 PRs, this time it's 3. What exactly is the PR limit, @dhruvmanila? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to some extent.
Can you review #L65-L77? Here are a few suggestions:
- float is a poor way of representing currency. First it introduces a lot of inefficient way when manipulating the money, I think its good you did first, multiply by 100 to take it to interger and even at that, i would have to do some round off, manupulate in integer form and convert back. Here's a typical example:
I got $2.99 and need to pay tax of .05 cents,
by working with floats, I would be doing 2.99*100 = 299, and .05*100=5,
money_less_tax = (299 - 5) / 100 = $2.94, in a fintech application with minute charges up 0.005 or so, could be worse with fractions and implicit manipulations, hence you should check out the decimal module
Refs:
(How to calculate money in python)[https://learnpython.com/blog/count-money-python/]
(Python Doc on why Decimal is better than float)[https://docs.python.org/3/library/decimal.html]
@50-Course This is a change with the algorithm itself. All this pr does is fix a bug and type errors in the algorithm, not refactor the code. |
It is 3 although it could happen that the event was posted late or there might've been some kind of latency issues or the request failed. I would just remove the limit actually, so if anyone's interested they can open a PR and set this variable to 0: https://github.com/TheAlgorithms/algorithms-keeper/blob/6d535abd7e5344606b979498ed68a756508792c5/algorithms_keeper/event/pull_request.py#L42-L43 |
…eAlgorithms#9009) * updating DIRECTORY.md * fix(get-amazon-product-data): Remove whitespace in headers * refactor(get-amazon-product-data): Don't print to_csv --------- Co-authored-by: github-actions <${GITHUB_ACTOR}@users.noreply.github.com>
Describe your change:
This PR fixes this error:
As well as some type changes and warnings (setting the featuresto
lxml
)Checklist: