-
Notifications
You must be signed in to change notification settings - Fork 203
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
Notebook protocol go-to-definition support #408
Conversation
e9fee6a
to
fab7d91
Compare
I think this minimally works, and the test passes now. |
Hey @jasongrout, nice to see you here! I took a quick look at PR #389 and it looks quite good, so I'll try to merge it next week. |
Awesome! @tkrabel-db and I are using python-lsp-server at Databricks for LSP support, and we are excited to contribute things like notebook protocol support that would be helpful for the whole community. As you saw on the other ticket, @tkrabel-db is eager to iterate on this repo to help maintain and improve the dev workflow as well. |
Great! That would be really helpful because I don't have much time lately to maintain this project. |
bb11bbc
to
a832b3c
Compare
I rebased this PR on top of the develop branch, now that #389 is merged. |
@jasongrout, this only requires to fix the code style issues reported by our failing workflow. The rest looks good to me. |
This simplifies the notebook-based go to definition
ce3afbd
to
c7708c4
Compare
I pushed some changes that address the pylint errors. The biggest change is factoring out extracting the cell data (pylint was complaining about too many local variables, as a hint that the complexity was too large). Since the black formatting PR was merged, of course there are conflicts. My hope is that running black on this pr will be enough to resolve those, though. |
@ccordoba12, @tkrabel-db - I think this is ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasongrout, please merge again with develop to get the changes done in PR #418 and see if your PR passes with them.
@jasongrout, please run Then I'll merge, unless @tkrabel-db has something else to say. |
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jasongrout!
@ccordoba12 - we're trying to figure out a plan for deploying the changes in this PR. Do you have a rough idea of a timeline for releasing 1.8.0? If it's going to be a while, would it be possible to do a 1.8.0 prerelease? What can we do to help with the 1.8.0 release? |
There are some minor issues that I'd like to address for 1.8.0, so if you could give me a hand with them, I'd really appreciate it. They have to do with preventing simple errors here and there. |
I actually didn't see this before I started going through, but I think I have PRs open now that should address all of these:
Since the first one requires a change on the json rpc side, it probably needs a version bump in the requirements there as well -- let me know if I can help with that. |
Thanks a lot @smacke for your help with this! I'll review your PRs tomorrow.
No worries, I'll take care of that when doing the release. |
No problem and awesome! Thanks a bunch Carlos. |
I just released 1.8.0. Thanks guys for all your help! |
This is a work-in-progress PR to add go to definition support on top of the notebook protocol developed in #389 by @tkrabel-db.
Likely each plugin, such as go to definition, will need to be modified to support the notebook syncing messages. We'll try to abstract out general patterns where possible, though.