falcon icon indicating copy to clipboard operation
falcon copied to clipboard

feat(routing): implement FloatConverter for #2022

Open meetshah133 opened this issue 3 years ago • 15 comments

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 and black 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.

meetshah133 avatar Feb 13 '22 11:02 meetshah133

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.

codecov[bot] avatar Feb 13 '22 12:02 codecov[bot]

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.

meetshah133 avatar Feb 13 '22 18:02 meetshah133

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

vytas7 avatar Feb 13 '22 18:02 vytas7

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 avatar Feb 14 '22 18:02 meetshah133

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

meetshah133 avatar Feb 14 '22 19:02 meetshah133

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

$ pip install -U black
$ black .

vytas7 avatar Feb 14 '22 19:02 vytas7

Thanks @vytas7 finally all checks passed

meetshah133 avatar Feb 14 '22 20:02 meetshah133

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

meetshah133 avatar Feb 15 '22 19:02 meetshah133

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

meetshah133 avatar Feb 17 '22 15:02 meetshah133

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

vytas7 avatar Mar 09 '22 08:03 vytas7

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.

meetshah133 avatar Mar 09 '22 11:03 meetshah133

Awesome, thanks for the update!

vytas7 avatar Mar 09 '22 11:03 vytas7

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

meetshah133 avatar Mar 12 '22 17:03 meetshah133

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

vytas7 avatar Mar 12 '22 17:03 vytas7

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!

kgriffs avatar Aug 05 '22 16:08 kgriffs