falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.51k stars 937 forks source link

feat(routing): implement FloatConverter for #2022 #2026

Closed meetshah133 closed 2 years ago

meetshah133 commented 2 years ago

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:

codecov[bot] commented 2 years ago

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.

meetshah133 commented 2 years ago

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.

vytas7 commented 2 years ago

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.)

meetshah133 commented 2 years ago

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.

meetshah133 commented 2 years ago

Hi @vytas7 , have implemented all the suggestions, need some guidance on "tox -e black" test failure

vytas7 commented 2 years ago

Luckily, that's the easiest one ;) Just install the latest black version into your environment, and run it:

$ pip install -U black
$ black .
meetshah133 commented 2 years ago

Thanks @vytas7 finally all checks passed

meetshah133 commented 2 years ago

@CaselIT and @vytas7 thanks for your inputs, I have created a new commit based on your inputs.

meetshah133 commented 2 years ago

Hi @vytas7 , @CaselIT I have worked on your suggestions and have implemented the changes. Could you please review it ?

vytas7 commented 2 years ago

Hi again @meetshah133, just checking if you are still working on these changes?

meetshah133 commented 2 years ago

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.

vytas7 commented 2 years ago

Awesome, thanks for the update!

meetshah133 commented 2 years ago

Hi @vytas7 , I have done the commit there are 3 test failing, but I am unable to understand why. Could you please assist ?

vytas7 commented 2 years ago

@meetshah133 could you please update your branch with the latest changes from master instead of force-pushing over?

kgriffs commented 2 years ago

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!