search

sindresorhus / ow

Function argument validation for humans
https://sindresorhus.com/ow/
MIT License
3.81k stars 108 forks source link

Add support for checking C style integer data types #120

Closed lukechilds closed 5 years ago

lukechilds commented 5 years ago

Resolves #118

Just opening this for some feedback, not ready to merge yet.

I've never used TypeScript before so have a few questions:

  1. I wrote byte-range as a separate lib but I'm getting errors because it's not written in TypeScript:
source/lib/predicates/number.ts:2:23 - error TS7016: Could not find a declaration file for module 'byte-range'. '/Users/lukechilds/dev/oss/ow/node_modules/byte-range/src/index.js' implicitly has an 'any' type.
  Try `npm install @types/byte-range` if it exists or add a new declaration (.d.ts) file containing `declare module 'byte-range';`

2 import byteRange from 'byte-range';

So looks like my options are add a definitions file or refactor it to use TypeScript. I know this module is advertised as being written in TypeScript so I'm assuming that would be a requirement of byte-range to be a dependency?

  1. I noticed all dependencies are under devDependencies.

Why is this? Do you want me to move byte-range to devDependencies too?

  1. Is there a way to add getters to the NumberPredicate class in a loop?

The current code for uint8 (https://github.com/sindresorhus/ow/commit/7134c23b53c4d6be1a78e1ecd1f7781483822f2c) will be practically identical for uint16, uint32, int8, int16 and int32. It would add way less code bloat to just loop over an array of ['uint8', 'uint16', 'uint32', 'int8', 'int16', 'int32'] and add the getters.

sindresorhus commented 5 years ago

I wrote byte-range as a separate lib but I'm getting errors because it's not written in TypeScript:

Yes, you need to include TypeScript definitions. See: https://github.com/sindresorhus/typescript-definition-style-guide I would be happy to review that if you want to give it a shot on your repo.

I noticed all dependencies are under devDependencies. Why is this? Do you want me to move byte-range to devDependencies too?

It's because we want ow to be a single file, to reduce require time, install time, and risk. This is because ow is intended to be a dependency for even small packages, so we don't want to be a bloat. So yes, move it to devDependencies.

Is there a way to add getters to the NumberPredicate class in a loop?

I have never tried. Maybe Object.defineProperty(NumberPredicate, currentRange) in a loop works?

sindresorhus commented 5 years ago

In this specific case, it would be easier to just hard-code the ranges. It's not like they will ever change. We already have them here: https://github.com/lukechilds/byte-range/blob/3d154a9af0f827dfbb44c5acf32029cf1a549ccb/test/unit.js#L11-L27

lukechilds commented 5 years ago

In this specific case, it would be easier to just hard-code the ranges. It's not like they will ever change. We already have them here: lukechilds/byte-range:test/unit.js@3d154a9#L11-L27

Just to clarify, you mean you don't want byte-range as a dependency? I should just hardcode the ranges in number.ts?

sindresorhus commented 5 years ago

Just to clarify, you mean you don't want byte-range as a dependency? I should just hardcode the ranges in number.ts?

It's not like I don't want it. Just seems it would be easier for you to implement it by just hard-coding the values, instead of having to write a TS definition for your package. Would also result in less bloat. Ow is already getting large.

lukechilds commented 5 years ago

Sure, I agree.

lukechilds commented 5 years ago

This should be all good now.

lukechilds commented 5 years ago

I just thought, there's no need to redo the integer/range logic, I can just piggyback on this.integer.inRange().

https://github.com/sindresorhus/ow/pull/120/commits/74c2f08f15ddf35b008d0f2d89e2cac4a6eb2927 gets rid of ~36 lines.

sindresorhus commented 5 years ago

Thanks, Luke :)