Skip to content
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

GetBool() method #2

Merged
merged 2 commits into from
Jun 1, 2018
Merged

GetBool() method #2

merged 2 commits into from
Jun 1, 2018

Conversation

mligor
Copy link
Contributor

@mligor mligor commented May 31, 2018

Hi András,

you made a great work here - anyway I needed reading bool from json and I implemented GetBool() method. I tried to follow your style.

If you want you can merge my changes in your repo

@codecov-io
Copy link

codecov-io commented May 31, 2018

Codecov Report

Merging #2 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #2   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         270    296   +26     
=====================================
+ Hits          270    296   +26
Impacted Files Coverage Δ
dyno.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15f794d...b5c37c3. Read the comment docs.

@icza
Copy link
Owner

icza commented May 31, 2018

My notes:

  • Your commit introduces dependencies to encoding/json and strconv packages. These could be avoided. For example my code tests for json.Number by checking if the type implements interface { Float64() (float64, error) } explicitly, to avoid having to refer to the json package.

  • GetXX functions that contain the short name of a type return the type without applying built-in conversion, such as GetInt(). When a method uses built-in conversion, to make this obvious, the long name of the type is used (e.g. GetInteger(), GetFloating()). So this GetBool() should not use built-in conversion, the one that uses should be called GetBoolean().

@mligor
Copy link
Contributor Author

mligor commented May 31, 2018

Thanks for the notes. I will try to solve it - I'm in my first week using golang :)
Also code coverage should not decrease.

- Renamed GetBool to GetBoolean
- Improved parsing string bool values (added support for Yes/No strings)
- Extended task cases to have 100% covarage
@icza icza merged commit 0a3d726 into icza:master Jun 1, 2018
@icza
Copy link
Owner

icza commented Jun 1, 2018

Accepted the PR. Made some subsequent commits to improve docs and consistency within the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants