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

fix: prevent setRawValue to convert 0 to empty string #116

Merged
merged 2 commits into from
Aug 6, 2018
Merged

fix: prevent setRawValue to convert 0 to empty string #116

merged 2 commits into from
Aug 6, 2018

Conversation

msuperina
Copy link
Contributor

Closes #113

@msuperina msuperina requested review from chrisprice and alisd23 July 16, 2018 15:03
@msuperina msuperina self-assigned this Jul 16, 2018
@ryanggrey
Copy link
Contributor

Would it be worth adding a test for this?

@msuperina
Copy link
Contributor Author

@ryanggrey I just added a single unit test for this use case.
Are you ok if we plan a separate PR to separate unit tests from e2e tests in the npm scripts, and then other PR to have some unit test coverage ?

@msuperina msuperina requested a review from ryanggrey July 25, 2018 14:27
value = helpers.rawToFormatted(val, this.options);
} else if (typeof val === 'string') {
value = val;
} else if (!val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this else-if clause if the clause above will fire instead? (empty string is typeof string, setting value to '')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is my understanding setRawValue is a public method, and can be called with undefined or null

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense to me

@oriondean oriondean merged commit a2019bf into ScottLogic:master Aug 6, 2018
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.

4 participants