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 incorrect typings for compile get/set methods #58

Merged
merged 1 commit into from Jan 21, 2022

Conversation

haakemon
Copy link
Contributor

@haakemon haakemon commented Jan 21, 2022

The previous PR (#54) was closed on incorrect assumptions. I added a comment on it, but got no response, so I hope a new PR is fine. We use this library, and the incorrect typings are annoying and I would like to see it fixed :)

So, let me try and clear up the confusion from the previous PR:

There is a get (https://github.com/janl/node-jsonpointer/blob/main/jsonpointer.js#L66) function that accepts 2 parameters, and
there is a set (https://github.com/janl/node-jsonpointer/blob/main/jsonpointer.js#L79) function that accept 3 parameters

Additionally, on the compile object
there is also a get (https://github.com/janl/node-jsonpointer/blob/main/jsonpointer.js#L89) method that accepts 1 parameter, and
there is also a set (https://github.com/janl/node-jsonpointer/blob/main/jsonpointer.js#L92) method that accepts 2 parameters.

Currently in the typings, both variants accept 2 and 3 parameters, which is incorrect.

This is also demonstrated in the tests, f.ex here: https://github.com/janl/node-jsonpointer/blob/main/test.js#L14 and here, on the compile object where only 1 parameter is used for get: https://github.com/janl/node-jsonpointer/blob/main/test.js#L126 and 2 parameters is used for set: https://github.com/janl/node-jsonpointer/blob/main/test.js#L127

This PR fixes the incorrect typings for the get/set methods on the compile object.

Thanks for the great library, I hope you will consider accepting this fix :)

@marcbachmann
Copy link
Collaborator

marcbachmann commented Jan 21, 2022

This looks good. Thanks for bringing this up again.

@marcbachmann marcbachmann merged commit b8e1e6a into janl:main Jan 21, 2022
5 checks passed
@marcbachmann
Copy link
Collaborator

marcbachmann commented Jan 22, 2022

I'd like to release that once #50 gets merged

@haakemon haakemon deleted the fix-typings branch Jan 22, 2022
@haakemon
Copy link
Contributor Author

haakemon commented Jul 13, 2022

Hi @marcbachmann , would it be possible to get this released without waiting for #50? There has not been any progress on that issue for some months now, and this typing issue is still bugging me 🙂

This was referenced Jul 13, 2022
@marcbachmann
Copy link
Collaborator

marcbachmann commented Jul 13, 2022

Released as v5.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants