falcon
falcon copied to clipboard
feat(routing): implement FloatConverter for #2022
Closes #2022
Changes:
Modified the existing logic of IntConverter class to reuse it in FloatConverter. Added he new converter to the list of Built-in Converters under the float identifier.
TODO address before merging:
- [x] Clean up code formatting and make the
pep8
andblack
Tox environments pass. - [x] Write missing tests for
FloatConverter
. We require 100% branch coverage at all times. - [x] Add
2022.newandimproved.rst
Towncrier newsfragment. - [x] Find a suitable replacement for
num_digits
. Or maybe we can remove it altogether? - [x] Do not use
camelCase
unless there is a compelling reason.
Codecov Report
Merging #2026 (590614f) into master (c76f442) will not change coverage. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## master #2026 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 62 62
Lines 6747 6766 +19
Branches 1255 1258 +3
=========================================
+ Hits 6747 6766 +19
Impacted Files | Coverage Δ | |
---|---|---|
falcon/routing/converters.py | 100.00% <100.00%> (ø) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
Hi @vytas7, can you please help by reviewing the code changes ? I have tested the FloatConverter and IntConverter class they seems working fine, Will be happy to get more inputs from you and contribute to the project.
Hi @meetshah133, and thanks for this PR! Unfortunately I won't have the bandwidth for this today, but will try to look at your code in the beginning of the next week. It also possible that @CaselIT, @kgriffs or other maintainers beat me to it :calendar:
To start with, you could address the CI failures (see "Some checks were not successful" below) to get the things moving faster :arrow_down: (It's lame that GitHub doesn't run Actions for first-time contributors, but I can try to at least push the button when I notice that you have committed new stuff.)
Thanks @vytas7 , @CaselIT , @myusko for your valuable inputs. I will work on those items suggested by you all and submit a new commit to this repository.
Hi @vytas7 , have implemented all the suggestions, need some guidance on "tox -e black" test failure
Luckily, that's the easiest one ;) Just install the latest black
version into your environment, and run it:
$ pip install -U black
$ black .
Thanks @vytas7 finally all checks passed
@CaselIT and @vytas7 thanks for your inputs, I have created a new commit based on your inputs.
Hi @vytas7 , @CaselIT I have worked on your suggestions and have implemented the changes. Could you please review it ?
Hi again @meetshah133, just checking if you are still working on these changes?
Hi again @meetshah133, just checking if you are still working on these changes?
Hi @vytas7, was not well for past few days. I am working on the changes suggested by you will soon commit the changes. Thanks.
Awesome, thanks for the update!
Hi @vytas7 , I have done the commit there are 3 test failing, but I am unable to understand why. Could you please assist ?
@meetshah133 could you please update your branch with the latest changes from master
instead of force-pushing over?
OK folks, in the interest of getting this over the finish line sooner rather than later, I took the liberty of trying to address the unresolved feedback. Please take another look. Thanks!