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.

Sign up for GitHub

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

Jump to bottom

Avoid deprecated API overwrite new API #2

Merged
merged 1 commit into from Nov 27, 2016
Merged

Avoid deprecated API overwrite new API #2

merged 1 commit into from Nov 27, 2016

Conversation

chemzqm
Copy link

@chemzqm chemzqm commented Nov 26, 2016 •
edited

Neovim have deprecated API, for example :

- method: false
  name: nvim_ui_attach
  parameters:
  - [Integer, width]
  - [Integer, height]
  - [Dictionary, options]
  return_type: void
  since: 1
- deprecated_since: 1
  method: false
  name: ui_attach
  parameters:
  - [Integer, width]
  - [Integer, height]
  - [Boolean, enable_rgb]
  return_type: void
  since: 0

Current implementation could overwrite the newer API with deprecated ones, like ui_attach

screen shot 2016-11-26 at 6 17 18 pm

@coveralls
Copy link

coveralls commented Nov 26, 2016 •
edited

Coverage Status

Coverage decreased (-7.8%) to 78.947% when pulling 963af82 on chemzqm:patch-1 into 36b6239 on rhysd:promisified.

rhysd
rhysd requested changes Nov 26, 2016
View reviewed changes
index.js
@@ -117,6 +117,7 @@ function generateWrappers(Nvim, types, metadata) {
if (typeName !== 'Nvim' && typeName !== 'Ui') {
method.metadata.parameterTypes.shift();
}
if (func.deprecated_since && Type.prototype.hasOwnProperty(methodName)) continue
Copy link
Owner

Choose a reason for hiding this comment

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

Could you fix style?

if (func.deprecated_since && Type.prototype.hasOwnProperty(methodName)) {
    continue;
}

Copy link
Author

Choose a reason for hiding this comment

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

sure

@rhysd
Copy link
Owner

rhysd commented Nov 26, 2016

@chemzqm

Thank you for catching it. Please check the review comment.

@chemzqm
Copy link
Author

chemzqm commented Nov 27, 2016 •
edited

@rhysd fixed, but I don't know how to make test get passed, the failure exists without this patch.

@coveralls
Copy link

coveralls commented Nov 27, 2016 •
edited

Coverage Status

Coverage decreased (-7.6%) to 79.13% when pulling ea3fb7e on chemzqm:patch-1 into 36b6239 on rhysd:promisified.

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.6%) to 79.13% when pulling 319f185 on chemzqm:patch-1 into 36b6239 on rhysd:promisified.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-7.6%) to 79.13% when pulling 319f185 on chemzqm:patch-1 into 36b6239 on rhysd:promisified.

@coveralls
Copy link

coveralls commented Nov 27, 2016 •
edited

Coverage Status

Coverage decreased (-7.6%) to 79.13% when pulling b95d8d5 on chemzqm:patch-1 into 36b6239 on rhysd:promisified.

@rhysd rhysd merged commit cbd1df3 into rhysd:promisified Nov 27, 2016
@chemzqm chemzqm deleted the patch-1 branch November 27, 2016 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhysd rhysd rhysd requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@chemzqm @coveralls @rhysd