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

improve code coverage to 99.31% #1373

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

ezkemboi
Copy link
Member

@ezkemboi ezkemboi commented Jul 4, 2020

Improve test coverage

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

if (xdata > new Date()) {
return false;
// eslint-disable-next-line max-len
} else if ((xdata.getFullYear() === yyyy) && (xdata.getMonth() === mm - 1) && (xdata.getDate() === dd)) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this rewrite?

Copy link
Member

Choose a reason for hiding this comment

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

Or check if you rebased correctly with the latest master.

Copy link
Member Author

Choose a reason for hiding this comment

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

xdata is created using yyyy, mm-1, and dd.
Getting xdata.getFullYear() and comparing it with what it was used to create it, it will always be true.
So, that line seemed redundant and of no use(it is like creating x from w, which is equal to 1 and comparing x.w with w or 1`.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks.

@profnandaa profnandaa merged commit 926accc into validatorjs:master Jul 7, 2020
@tux-tn
Copy link
Member

tux-tn commented Jul 11, 2020

I did the same work to improve the coverage to 100% how can i create a new pull request without erasing @ezkemboi work ? :(

@ezkemboi
Copy link
Member Author

@tux-tn, I think if you rebase from the master, it will definitely not erase. Also, there are some code changes that I made on isIdentityCard().

@tux-tn
Copy link
Member

tux-tn commented Jul 11, 2020

@ezkemboi my issue was that i already commited my changes last week and didn't see your PR. But fixed, i already submitted my PR including your changes. Feel free to check #1376 and review it, especially the changes to isIdentityCard

@ezkemboi
Copy link
Member Author

ezkemboi commented Jul 11, 2020 via email

@profnandaa profnandaa mentioned this pull request Sep 18, 2020
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.

3 participants