-
-
Notifications
You must be signed in to change notification settings - Fork 46.7k
Refactor currency_converter.py #5917
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
Conversation
Co-authored-by: xcodz-dot <[email protected]>
Co-authored-by: xcodz-dot <[email protected]>
Tests are failing because of a problem the way the CI is set up. I have the same issue locally on other projects. On a fresh run, there is no cache. So when Mypy trieds to install types, it doesn't know what to install for https://github.com/TheAlgorithms/Python/runs/4831403253?check_suite_focus=true
A flow what does work but is inconvenient is:
What I prefer to is install the types directly in requirements-dev.txt or like this, then run Mypy. pip install types-requests
mypy --ignore-missing-imports --non-interactive . This is more manual but less magical (you know exactly what types you install and even the version if you pin it) and lets you run Mypy without the cache or the install flag. |
|
Thanks that worked locally. I added a PR for just that |
I am getting an error locally on the type of params being a dict when it should be But I can't get this to work as my import is not right. Maybe mean to be from request stubs? I haven't gone this deep into types and don't have capacity at the moment to figure it out
|
thanks! |
Describe your change:
The use of locals in the function is unnecessary. It makes the code harder to follow because you have to check what goes in it, the locals line doesn't save any lines, and the rename of
from_
tofrom
has no benefit.Also the types can be inferred from constants so no need to say 1.0 is a float.
Checklist:
Fixes: #{$ISSUE_NO}
.