Skip to content

[color] update types for color package to 2.0 #18389

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

Merged
merged 2 commits into from
Aug 1, 2017

Conversation

Ailrun
Copy link
Contributor

@Ailrun Ailrun commented Jul 25, 2017

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: <https://github.com/Qix-/color/blob/master/package.json>
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@dt-bot
Copy link
Member

dt-bot commented Jul 25, 2017

types/color/index.d.ts

to author (@LKay). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

types/color/v1/index.d.ts

Checklist

@Ailrun Ailrun closed this Jul 25, 2017
@Ailrun Ailrun reopened this Jul 25, 2017
@Ailrun Ailrun changed the title [color] create types for color package [color] update types for color package to 2.0 Jul 25, 2017
@typescript-bot typescript-bot added the New Definition This PR creates a new definition package. label Jul 25, 2017
@typescript-bot
Copy link
Contributor

This PR has been open and unchanged 5 days without signoff or complaint. This will be merged by a maintainer soon if there are no objections.

@typescript-bot typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Jul 30, 2017

type ColorParam = string | { [param: string]: number } | number;
declare class Color {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect representation of this library as you the constructors are essentially call signatures (https://github.com/Qix-/color#constructors) Since this doesnt mention that the constructors need to follow new Color you cannot declare Color as a class

Copy link
Contributor Author

@Ailrun Ailrun Aug 1, 2017

Choose a reason for hiding this comment

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

@sheetalkamat
But it can also use with new. Is there any correct way to model callable class?

Copy link
Contributor Author

@Ailrun Ailrun Aug 1, 2017

Choose a reason for hiding this comment

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

@sheetalkamat
In the previous version of type definition, new keyword make type error, where as color library itself works well with new keyword.

And, by its source code(https://github.com/Qix-/color/blob/master/index.js#L28), library author surely means class, not just something like callable one. (he use new for Color in his code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sheetalkamat
If I read this issue correctly, there's no way to modeling callable class in TypeScript, isn't it?
Then I believe class is better than interface, i.e., I want more concrete reason for revision.
Or, is interface better than class? Then, could you give me some reasoning related with those?

@sheetalkamat sheetalkamat added the Revision needed This PR needs code changes before it can be merged. label Jul 31, 2017
@Ailrun
Copy link
Contributor Author

Ailrun commented Aug 1, 2017

@sheetalkamat I fix it. Now const Color = require('color') can be used with new and also is callable.

b(): number;
b(val: number): Color;
keyword(): string;
keyword(val: string): Color;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am looking at this and wondering why does val have to be string. https://github.com/Qix-/color/blob/master/index.js#L232 wouldnt it mean that val can be of type ColorParam? and not just string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sheetalkamat
It's just because I try to match return type of getter and parameter type of (immutable) setter.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ailrun I am not familiar with the API and since the PR is open for more than 7 days, I will merge it if you think this is correct parameter type.

Copy link
Contributor Author

@Ailrun Ailrun Aug 1, 2017

Choose a reason for hiding this comment

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

@sheetalkamat At least I believe, using keyword function with any other parameter than css keywords is somehow insane thing, so I argue that this one is a valid type.

@sheetalkamat sheetalkamat merged commit ec1669c into DefinitelyTyped:master Aug 1, 2017
milosdanilov pushed a commit to milosdanilov/DefinitelyTyped that referenced this pull request Aug 5, 2017
* create types for color package

* Fix Color class to interface
@Ailrun Ailrun deleted the color branch July 31, 2018 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Definition This PR creates a new definition package. Revision needed This PR needs code changes before it can be merged. Unmerged The author did not merge the PR when it was ready.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants