Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

fix: edge cases for integers flags #50

Merged
merged 3 commits into from
May 28, 2019
Merged

fix: edge cases for integers flags #50

merged 3 commits into from
May 28, 2019

Conversation

tbrannam
Copy link
Contributor

@tbrannam tbrannam commented May 3, 2019

validateFlags was not differentiating between 0 and undefined which prevented 0 from being accepted as an input for integer based flags.

integer did not include optional leading - preventing negative numbers from being accepted

`validateFlags` was not differentiating between `0` and `undefined` which prevented `0` from being accepted as an input for integer based flags.

`integer` did not include optional leading `-` preventing negative numbers from being accepted
@salesforce-cla
Copy link

salesforce-cla bot commented May 3, 2019

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Todd Brannam <t***@h***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce.com Contributor License Agreement and this Pull Request will be revalidated.

@codecov
Copy link

codecov bot commented May 3, 2019

Codecov Report

Merging #50 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   91.18%   91.35%   +0.17%     
==========================================
  Files          11       11              
  Lines         329      324       -5     
  Branches       89       85       -4     
==========================================
- Hits          300      296       -4     
+ Misses         13       12       -1     
  Partials       16       16
Impacted Files Coverage Δ
src/validate.ts 96.87% <100%> (+2.28%) ⬆️
src/flags.ts 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 265a73f...ca11405. Read the comment docs.

@keawade
Copy link

keawade commented May 7, 2019

Oh man, I opened #51 just a few hours after this! 😆I didn't see anything in the issues and didn't think to check the PRs, sorry!

Could you add the additional tests I wrote to verify negative numbers and known failure cases (fractions and decimals) to this PR? Then I can close my PR.

@tbrannam
Copy link
Contributor Author

tbrannam commented May 7, 2019 via email

Added tests from PR #51 to set edge cases for integer parsing
@tbrannam
Copy link
Contributor Author

tbrannam commented May 7, 2019

@keawade added your tests in

@keawade
Copy link

keawade commented May 24, 2019

@jdxcode, can we get a review please? 😄

Copy link

@keawade keawade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@tbrannam
Copy link
Contributor Author

@jdxcode seems to have left Heroku, are you still caring for this baby?

@keawade
Copy link

keawade commented May 28, 2019

@mattgraham maybe? He's the only publicly listed member of the oclif org.

@jdx
Copy link
Contributor

jdx commented May 28, 2019

I'm no longer working on this project. @RasPhilCo and @elbandito should be able to assist

Copy link
Contributor

@RasPhilCo RasPhilCo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix! 🏄

@RasPhilCo RasPhilCo changed the title Fix edge cases for integers flags fix: edge cases for integers flags May 28, 2019
@RasPhilCo RasPhilCo merged commit f233ee5 into oclif:master May 28, 2019
@tbrannam tbrannam deleted the validate-zero-int branch May 28, 2019 21:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants