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

feat: add isLatLong, isLatitude, isLongtitude validators #427

Merged
merged 10 commits into from
Oct 2, 2019
Merged

feat: add isLatLong, isLatitude, isLongtitude validators #427

merged 10 commits into from
Oct 2, 2019

Conversation

rubiin
Copy link
Contributor

@rubiin rubiin commented Sep 30, 2019

added isLatLong() which is used to check whether a given set of points is latitude and longitude or not. The validation is taken from validator.js

Copy link
Contributor

@vlapo vlapo left a comment

Choose a reason for hiding this comment

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

Also please update docs

src/validation/Validator.ts Show resolved Hide resolved
@vlapo vlapo changed the title added islatlong feat: add islatlong from validatorjs Oct 1, 2019
@vlapo vlapo changed the title feat: add islatlong from validatorjs feat: add isLatLong from validatorjs Oct 1, 2019
@vlapo
Copy link
Contributor

vlapo commented Oct 1, 2019

I was thinking about this and maybe we should also add @IsLatitude and @IsLongtitude validators. I am pretty sure there are developers which want to separate lat and long and use object representation instead of "lat,long" string. We can use validatorJs.isLatLong() e.g. validatorJs.isLatLong(lat+',0)

Also please could you ad default error messages for all new validators.

@rubiin
Copy link
Contributor Author

rubiin commented Oct 1, 2019

Okay sure. About the islatitude and islongitude, I can work it in another or starting tomorrow

@rubiin
Copy link
Contributor Author

rubiin commented Oct 1, 2019

@vlapo added the default error message. sorry about that

@vlapo
Copy link
Contributor

vlapo commented Oct 1, 2019

Please add it in this PR and thanks for your contributions.

@rubiin
Copy link
Contributor Author

rubiin commented Oct 2, 2019

@vlapo I have done it please check

@vlapo
Copy link
Contributor

vlapo commented Oct 2, 2019

What have you done? I do not see @IsLatitude and @IsLongtitude validators in your changes.

@rubiin
Copy link
Contributor Author

rubiin commented Oct 2, 2019

added the default message for the pr above. I was thinking of separating the islongitude and islatitude in another PR @vlapo or do you want me to do that in this pr itself?

@vlapo
Copy link
Contributor

vlapo commented Oct 2, 2019

No, please do it in this PR.

@rubiin
Copy link
Contributor Author

rubiin commented Oct 2, 2019

okay sure. I will ping you after that

@rubiin
Copy link
Contributor Author

rubiin commented Oct 2, 2019

@vlapo its complete

Copy link
Contributor

@vlapo vlapo left a comment

Choose a reason for hiding this comment

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

And again, docs and default error messages...

src/validation/Validator.ts Outdated Show resolved Hide resolved
src/validation/Validator.ts Outdated Show resolved Hide resolved
src/validation/Validator.ts Outdated Show resolved Hide resolved
@rubiin
Copy link
Contributor Author

rubiin commented Oct 2, 2019

I have fixed all the issues that you have mentioned @vlapo

@rubiin rubiin requested a review from vlapo October 2, 2019 14:32
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated
Comment on lines 927 to 928
| `@IsLatitude()` | check if the string is a valid latitude coordinate
| `@IsLongitude()` | check if the string is a valid longitude coordinate
Copy link
Contributor

@vlapo vlapo Oct 2, 2019

Choose a reason for hiding this comment

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

  1. For @IsLatitude and @IsLongitude - string or number - check if the string or number is a valid latitude/longitude coordinate

  2. Again formatting. There is more spaces than is necessary. Should be:

| `@IsLatLong()`                                  | check if the string is a valid latitude-longitude coordinate in the format lat,long
| `@IsLatitude()`                                 | check if the string is a valid latitude coordinate
| `@IsLongitude()`                                | check if the string is a valid longitude coordinate

src/validation/ValidationTypes.ts Outdated Show resolved Hide resolved
src/validation/ValidationTypes.ts Outdated Show resolved Hide resolved
Comment on lines 493 to 494
const validValues = ["85.3446311", "85.2100893"];
const invalidValues = ["853446311", "as.as12"];
Copy link
Contributor

Choose a reason for hiding this comment

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

Add also valid/invalid number values.

src/validation/Validator.ts Outdated Show resolved Hide resolved
src/validation/Validator.ts Outdated Show resolved Hide resolved
@vlapo vlapo changed the title feat: add isLatLong from validatorjs feat: add isLatLong, isLatitude, isLongtitude validators Oct 2, 2019
@rubiin rubiin requested a review from vlapo October 2, 2019 15:01
Comment on lines 357 to 358


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra lines between comment and function - why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just wanted to make it look clear on my ide

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry but extra empty lines between comment and function related directly to comment do not look clearly. It is pointless :) Check another code in class-validator and try to use same formatting as we use before in our code base.

/**
* Checks if a given value is a longitude.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra lines between comment and function - why?


isLongitude(value: any): boolean {
return (typeof value === "number" || this.isString(value)) && this.isLatLong(`${value},0`);

Copy link
Contributor

Choose a reason for hiding this comment

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

Another extra line - why?

@rubiin rubiin requested a review from vlapo October 2, 2019 15:29
Copy link
Contributor

@vlapo vlapo left a comment

Choose a reason for hiding this comment

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

@rubiin why did you format all code?!? Format only code you change/add/edit!

@rubiin rubiin requested a review from vlapo October 2, 2019 15:46
@rubiin
Copy link
Contributor Author

rubiin commented Oct 2, 2019

my editor formated those so i rerolled and back to the previous commit and only formated my code

@rubiin
Copy link
Contributor Author

rubiin commented Oct 2, 2019

@vlapo fixed

Copy link
Contributor

@vlapo vlapo left a comment

Choose a reason for hiding this comment

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

I have small formatting issues. But be honest, I do not have energy to request changes again. I will fix it in master. Thank you for your contribution. But please, next time be more precise.

@vlapo vlapo merged commit 3fd15c4 into typestack:master Oct 2, 2019
@yogevlahyani
Copy link
Contributor

yogevlahyani commented Oct 8, 2019

When is this version going to be published to npm?

@vlapo
Copy link
Contributor

vlapo commented Oct 9, 2019

Hope this weekend :)

@DrNightmare
Copy link

DrNightmare commented Jun 12, 2020

Hi @vlapo and @rubiin
3fd15c4#diff-288d846ec06621fddacbd61e08486017R284

The IsLatitude function is using the IS_LONGITUDE ValidationType, and vice versa.

Because of this, the IsLatitude() check returns the following (incorrect) error message:

"$property must be a longitude string or number"

@github-actions
Copy link

github-actions bot commented Aug 3, 2020

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants