[@types/validator] Add no_symbols as option #28600

henrikra · 2018-09-03T12:46:10Z

Types for validator was missing options for function isNumeric

Here is picture of documentation for isNumeric

Here is also implementation for that function: https://github.com/chriso/validator.js/blob/c192d7c442193d790f5497d92267931d48b2dbc8/src/lib/isNumeric.js

henrikra · 2018-09-03T13:11:52Z

The CI is failing but it is not this library it is sequelize

henrikra · 2018-09-03T13:13:26Z

@qqilihq @builtinnya Can you guys check this out?

typescript-bot · 2018-09-03T13:25:57Z

@henrikra Thank you for submitting this PR!

🔔 @tgfjt @chrootsu @IOAyman @louy @kacepe @deptno @builtinnya @qqilihq - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

typescript-bot · 2018-09-03T13:25:59Z

@henrikra The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

qqilihq · 2018-09-03T16:42:48Z

types/validator/index.d.ts

@@ -177,7 +177,7 @@ declare namespace ValidatorJS {
    isMultibyte(str: string): boolean;

    // check if the string contains only numbers.
-    isNumeric(str: string): boolean;
+    isNumeric(str: string, options?: { no_symbols?: boolean }): boolean;


I would suggest introducing an interface IsNumericOptions in order to adhere to the current structure.

qqilihq · 2018-09-03T16:43:19Z

types/validator/validator-tests.ts

@@ -565,6 +565,7 @@ let any: any;
  result = validator.isMultibyte('sample');

  result = validator.isNumeric('sample');
+  result = validator.isNumeric('+358', { no_symbols: true })


Add trailing ;

henrikra · 2018-09-03T18:40:59Z

What about now @qqilihq ?

Btw one thing that which came to my mind is that should we also export interfaces of different options? Because for example library called class-validator is using validator library as dependency and they are redefining types in this package. For example IsCurrencyOptions on this package:

DefinitelyTyped/types/validator/index.d.ts

Line 280 in 5344bfc

interface IsCurrencyOptions {

and on class-validator are exactly the same: https://github.com/typestack/class-validator/blob/a4013a70d025463a2c431273e728a580645ab8dc/src/validation/ValidationTypeOptions.ts#L12. I think this isn't that great. I think the solution would be to export interface from this package so other packages could use them :). What do you think?

typescript-bot · 2018-09-03T19:22:55Z

@henrikra The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

qqilihq · 2018-09-03T19:33:38Z

@henrikra Looks good to me 👍

About the failing build: Not sure what's going on here. I'm just a contributor to this specific package, so I've no clue, why the CI is failing. But it's obviously not caused by your changes, so I'd suggest getting in touch with one of the Definitely Typed admins/maintainers, and/or opening a separate issue about it.

henrikra · 2018-09-03T19:38:09Z

Good to hear. Did you get what I was trying to say with exporting options interfaces?

qqilihq · 2018-09-04T10:53:50Z

Did you get what I was trying to say with exporting options interfaces?

@henrikra Sorry, just had time to have a close look/thought. I'd agree with you that exporting would make sense. But I'm not a TS/DT expert, so I'd suggest waiting for further opinions!

henrikra · 2018-09-06T12:07:22Z

@builtinnya Can you check this out?

builtinnya · 2018-09-07T04:07:44Z

@henrikra
LGTM 👍
The same error as CI can be reproduced on current master (2ae8bc7) by running npm run lint sequelize.
It should have nothing to do with your change.
I agee to exporting options interfaces.

henrikra · 2018-09-07T05:04:59Z

@builtinnya I can add those exports to this PR too if it is ok?

builtinnya · 2018-09-07T06:04:10Z

@henrikra
I'd recommend that you add those exports in a separated PR for clarity.
I will review your PR then.

henrikra · 2018-09-07T06:42:24Z

Ok I can do that :) Can we merge this?

builtinnya · 2018-09-07T07:13:01Z

As @qqilihq suggested, we need to get in touch with one of maintainers OR resolve the build error.
Some pull requests based on 2ae8bc7 have passed CI, so this PR may also pass if you re-run the tests.
Can you rebase your commits to trigger the CI again?

henrikra · 2018-09-07T08:16:15Z

@builtinnya I did rebase new master to my PR but still it has the same error.

@IOAyman @uniqueiniquity Can check this out so this can be merged?

typescript-bot · 2018-09-07T08:25:14Z

@henrikra The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

bhanuy · 2018-09-07T09:32:03Z

tested and works for me..

typescript-bot · 2018-09-13T08:27:20Z

@henrikra I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

henrikra · 2018-09-13T09:38:19Z

@IOAyman @mhegazy @minestarks Can you guys check this and merge? The issue with CI is not made by this PR!

uniqueiniquity · 2018-09-13T15:37:44Z

@henrikra I'm rerunning the build, let's see if that fixes anything.

henrikra · 2018-09-13T18:50:45Z

@uniqueiniquity As you can see this has something to do with sequelize and my code cannot change that

So can't we just merge this or?

uniqueiniquity · 2018-09-13T20:21:23Z

@henrikra Sorry about the delay. After discussing with @RyanCavanaugh, I'm going to go fix the other package and make sure you get a clean run before merging this. Thanks for your patience here.

henrikra · 2018-09-14T05:23:41Z

Thank you for merging!

@qqilihq @builtinnya Next I am going to make that PR which exports those interfaces so other libraries using validator can use those types too so they don't have to replicate them :)

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Sep 3, 2018

henrikra mentioned this pull request Sep 3, 2018

How to use isNumeric with options? typestack/class-validator#253

Closed

qqilihq suggested changes Sep 3, 2018

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Sep 3, 2018

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Sep 3, 2018

Add no_symbols as option

0435a0c

Add fixes

05eaa44

uniqueiniquity closed this Sep 13, 2018

uniqueiniquity reopened this Sep 13, 2018

uniqueiniquity mentioned this pull request Sep 13, 2018

Add PromiseLike-compatible type for then #28866

Merged

9 tasks

ghost merged commit 35640ad into DefinitelyTyped:master Sep 13, 2018

This was referenced Sep 14, 2018

[@types/validator] Export interfaces from namespace too #28890

Closed

Remove duplicated types typestack/class-validator#262

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[@types/validator] Add no_symbols as option #28600

[@types/validator] Add no_symbols as option #28600

henrikra commented Sep 3, 2018 •

edited

Loading

henrikra commented Sep 3, 2018

henrikra commented Sep 3, 2018

typescript-bot commented Sep 3, 2018 •

edited

Loading

typescript-bot commented Sep 3, 2018 •

edited

Loading

qqilihq Sep 3, 2018

qqilihq Sep 3, 2018

henrikra commented Sep 3, 2018 •

edited

Loading

typescript-bot commented Sep 3, 2018 •

edited

Loading

qqilihq commented Sep 3, 2018

henrikra commented Sep 3, 2018

qqilihq commented Sep 4, 2018 •

edited

Loading

henrikra commented Sep 6, 2018

builtinnya commented Sep 7, 2018

henrikra commented Sep 7, 2018

builtinnya commented Sep 7, 2018

henrikra commented Sep 7, 2018

builtinnya commented Sep 7, 2018 •

edited

Loading

henrikra commented Sep 7, 2018

typescript-bot commented Sep 7, 2018 •

edited

Loading

bhanuy commented Sep 7, 2018

typescript-bot commented Sep 13, 2018

henrikra commented Sep 13, 2018

uniqueiniquity commented Sep 13, 2018

henrikra commented Sep 13, 2018 •

edited

Loading

uniqueiniquity commented Sep 13, 2018

henrikra commented Sep 14, 2018

[@types/validator] Add no_symbols as option #28600

[@types/validator] Add no_symbols as option #28600

Conversation

henrikra commented Sep 3, 2018 • edited Loading

henrikra commented Sep 3, 2018

henrikra commented Sep 3, 2018

typescript-bot commented Sep 3, 2018 • edited Loading

typescript-bot commented Sep 3, 2018 • edited Loading

qqilihq Sep 3, 2018

Choose a reason for hiding this comment

qqilihq Sep 3, 2018

Choose a reason for hiding this comment

henrikra commented Sep 3, 2018 • edited Loading

typescript-bot commented Sep 3, 2018 • edited Loading

qqilihq commented Sep 3, 2018

henrikra commented Sep 3, 2018

qqilihq commented Sep 4, 2018 • edited Loading

henrikra commented Sep 6, 2018

builtinnya commented Sep 7, 2018

henrikra commented Sep 7, 2018

builtinnya commented Sep 7, 2018

henrikra commented Sep 7, 2018

builtinnya commented Sep 7, 2018 • edited Loading

henrikra commented Sep 7, 2018

typescript-bot commented Sep 7, 2018 • edited Loading

bhanuy commented Sep 7, 2018

typescript-bot commented Sep 13, 2018

henrikra commented Sep 13, 2018

uniqueiniquity commented Sep 13, 2018

henrikra commented Sep 13, 2018 • edited Loading

uniqueiniquity commented Sep 13, 2018

henrikra commented Sep 14, 2018

henrikra commented Sep 3, 2018 •

edited

Loading

typescript-bot commented Sep 3, 2018 •

edited

Loading

typescript-bot commented Sep 3, 2018 •

edited

Loading

henrikra commented Sep 3, 2018 •

edited

Loading

typescript-bot commented Sep 3, 2018 •

edited

Loading

qqilihq commented Sep 4, 2018 •

edited

Loading

builtinnya commented Sep 7, 2018 •

edited

Loading

typescript-bot commented Sep 7, 2018 •

edited

Loading

henrikra commented Sep 13, 2018 •

edited

Loading