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

added feature and test for zipcode validation #26

Merged
merged 3 commits into from
Oct 2, 2020
Merged

added feature and test for zipcode validation #26

merged 3 commits into from
Oct 2, 2020

Conversation

xxKeefer
Copy link
Contributor

@xxKeefer xxKeefer commented Sep 30, 2020

Description

Added feature and test for validating zip codes to utils

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • [ X] New feature (non-breaking change which adds functionality)

Checklist:

  • I checked that there's no dataset update (can be done by running npm run update-dataset)
  • npm test passes on my machine
  • npm run lint passes on my machine

* @returns {Boolean}
*/
const isValidZipCode = (code) =>
Number.isInteger(code) && code >= 1000 && code <= ZIP_COUNT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your point in #24 is valid IMO, we can pass a string like '01027', so I'd suggest either just checking if code is not NaN (with Number.isNaN), or you use parseInt(code) then you check for parsedCode >= 1000 && parsedCode <= ZIP_COUNT;

@InternetRamen
Copy link

What should I do? I'm new to open source.

@Fcmam5
Copy link
Collaborator

Fcmam5 commented Sep 30, 2020

What should I do? I'm new to open source.

You can help us here by reviewing the code, and suggest any improvements. But first, we should wait for the PR author to publish the latest changes and then when the review is requested we can check it.
Or sometimes we notice something that we can improve and it could be outside of this PR scope, you can resolve it in a separate PR.

But for now, everything is fine :) just check our latest issue, it should be a nice welcoming-challenge for your open-source journey 🙂 !

@xxKeefer
Copy link
Contributor Author

xxKeefer commented Oct 1, 2020

Description

Added feature and test for validating zip codes to utils
function now handles valid strings that can be parsed into into a valid integer, updated test to reflect this.

Fixes #26

Type of change

Please delete options that are not relevant.

[ X] New feature (non-breaking change which adds functionality)

Checklist:

[ X] I checked that there's no dataset update (can be done by running npm run update-dataset)
[ X] npm test passes on my machine
[X ] npm run lint passes on my machine

Copy link
Collaborator

@Fcmam5 Fcmam5 left a comment

Choose a reason for hiding this comment

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

Clean! Thank you very much 😄

I only have the follow suggestions.

  • please export this file in index.js and document it in README.md

src/utils/isValidZipCode.js Outdated Show resolved Hide resolved
src/utils/isValidZipCode.js Outdated Show resolved Hide resolved
@xxKeefer
Copy link
Contributor Author

xxKeefer commented Oct 2, 2020

OK, should be good now, can you please look over the documentation in the README, i tried to follow the format but i am not sure if i matched it correctly.

Copy link
Collaborator

@Fcmam5 Fcmam5 left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you very much @xxKeefer for your contribution.

@Fcmam5 Fcmam5 linked an issue Oct 2, 2020 that may be closed by this pull request
@Fcmam5 Fcmam5 merged commit c3cbfc8 into dzcode-io:develop Oct 2, 2020
@Fcmam5
Copy link
Collaborator

Fcmam5 commented Oct 2, 2020

@all-contributors add @xxKeefer for code

@allcontributors
Copy link
Contributor

@Fcmam5

I've put up a pull request to add @xxKeefer! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Released
Development

Successfully merging this pull request may close these issues.

Fix: Do not search for wilaya by zip code if it's invalid
3 participants