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

Add explicit API function definitions #493

Closed
wants to merge 1 commit into from

Conversation

stevearc
Copy link
Contributor

Depends on #492

With the addition of type annotations, there is now a reason to explicitly enumerate the API methods instead of relying on the magic __getattr__. If they are explicitly provided, plugin authors will get better autocompletion (via LSP) and more type safety guarantees.

Since this depends on #492, it has all of its commits. I'll rebase it once merged, but if anyone knows of a better Github workflow for dependent PR's, please let me know.

@lgtm-com
Copy link

lgtm-com bot commented Jul 29, 2021

This pull request introduces 2 alerts when merging 51fef93 into 581cba4 - view on LGTM.com

new alerts:

  • 2 for `__init__` method calls overridden method

@justinmk
Copy link
Member

justinmk commented Aug 27, 2021

Is there a test that checks the types against the actual nvim --api-info schema? That is a minimum requirement imo.

Also would be great if we could generate this instead of manually maintaining it. Then we could have a CI bot that sends PRs automatically.

@stevearc
Copy link
Contributor Author

@justinmk That's a great idea! I'm happy to take a crack at auto-generation, or at least auto-verification. The PR this depends on is just collecting dust at the moment and I haven't seen much indication that it will be merged, so I'll probably defer the work until after there's more progress there.

@justinmk
Copy link
Member

justinmk commented Aug 29, 2021

I'm happy to take a crack at auto-generation, or at least auto-verification

just auto-verification is enough to move forward. I would even say, let's wait on auto-generation until we get this first phase merged.

I think we're ready to move forward with #492 (comment)

This mostly doesn't change the functionality, as most these functions
were already available via the `__getattr__` magic methods (exceptions
for things like Window.cursor and Nvim.echo). The goal of this change is
to provide proper type annotations for plugin authors.
@justinmk
Copy link
Member

Rebased after merging #492.

Failing Windows CI is a known issue, unrelated to this PR.

@justinmk justinmk changed the title [Blocked] Add explicit API function definitions Add explicit API function definitions Jul 14, 2023
@justinmk
Copy link
Member

Not in favor of this, for same reasoning as in neovim/node-client#199 (comment) : pynvim is a useful "reference implementation" Neovim client, but we need to be ruthless about keeping it extremely low-maintenance, because of the lack of ownership in the last 2+ years.

Most APIs should be autogenerated at runtime, if at all. Meanwhile, request('nvim_...') is always available to clients.

@justinmk justinmk closed this Jul 14, 2023
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.

2 participants