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

fix types #535

Merged
merged 3 commits into from
Aug 9, 2018
Merged

fix types #535

merged 3 commits into from
Aug 9, 2018

Conversation

hipstersmoothie
Copy link
Collaborator

What's Changing and Why

code review from @VojtechStep

What else might be affected

just types

Tasks

  • Add tests
  • Update Documentation
  • Update jimp.d.ts
  • Add SemVer Label

@hipstersmoothie hipstersmoothie added the bug there is a bug in the way jimp behaves label Aug 9, 2018
@hipstersmoothie
Copy link
Collaborator Author

hipstersmoothie commented Aug 9, 2018

@VojtechStep any idea how to fix the buffer thing?

this could probably close #459

cb: GenericCallback<number, T, Jimp>
): this;
cb: GenericCallback<number, any, Jimp>
): number;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should actually return void

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this one for sure returns a number though

Copy link
Contributor

Choose a reason for hiding this comment

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

Well the function doesn't have a return statement

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you're reading the diff wrong. line 523 is a part of rgbaToInt not appendConstructorOption

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh 100% you're right, sorry about that 🤦‍♂️

jimp.d.ts Outdated
rotate(
deg: number,
mode: number | boolean | undefined | null,
cb?: Jimp.ImageCallback<T>
mode: number | boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems I've made a mistake before, the type of mode should be string | boolean and it should be optional

jimp.d.ts Outdated
composite(
src: Jimp,
x: number,
y: number,
options: Jimp.BlendModel,
cb?: Jimp.ImageCallback<T>
options: Jimp.BlendMode,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we provide a default value for options, it should be marked as optional

@VojtechStep
Copy link
Contributor

@hipstersmoothie Regarding the Buffer thing, I'm not sure how to approach it. We should add @types/node as a dependency of some sort, but there are drawbacks:

  • Regular dependencies: won't go here, types are only for development
  • DevDependencies: If we put it here, they are installed even for users of the library who don't use TypeScript
  • OptionalDependencies: They still get installed
  • PeerDependencies: Users don't need to install the types, but the will get annoying messages in the terminal if they don't

@hipstersmoothie
Copy link
Collaborator Author

Hmm. Better left to a different PR then. May just have to put something in the readme on how to configure it properly

@hipstersmoothie hipstersmoothie merged commit 2a2cdea into jimp-dev:master Aug 9, 2018
@hipstersmoothie hipstersmoothie deleted the types branch August 9, 2018 10:11
hipstersmoothie added a commit to hipstersmoothie/jimp that referenced this pull request Aug 10, 2018
* fix types

* fix more build errors

* code reviews
@nrkn
Copy link
Contributor

nrkn commented Aug 23, 2018

You forgot to make this callback optional:

        static rgbaToInt(
            r: number,
            g: number,
            b: number,
            a: number,
            cb: GenericCallback<number, any, Jimp>
        ): number;

@hipstersmoothie
Copy link
Collaborator Author

@nrkn please make a pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug there is a bug in the way jimp behaves
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants