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

Support JupyterLab 2.2 #301

Merged
merged 16 commits into from
Aug 11, 2020
Merged

Support JupyterLab 2.2 #301

merged 16 commits into from
Aug 11, 2020

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Jul 29, 2020

References

Supersedes #239, fixes #241.

Code changes

  • adopted to ICompletionItem interface
  • implements ICompletionItemsConnector over simple DataConnector
  • uses kernel icon for suggestions from kernel if type is not available
  • passes documentation and deprecation note to the completer

User-facing changes

In addition to the new shiny completer widget (see all three PRs for jupyterlab/jupyterlab#7983):

  • the autocomplete is no longer overaggressive (Only Autoinsert Single Completion Items on Tab jupyterlab/jupyterlab#8375) and does not cause path-completion issues - this opens way to implementing hitlander mode
    • this means that pressing dot . once does not cause autocompletion to insert text if only one item was found. Pressing tab twice does the trick.
  • we get label rather than insertText displayed

competer-opt

Backwards-incompatible changes

ICompletionItemsConnector is a breaking change and not backwards compatible with JupyterLab 2.1

Chores

  • linted
  • tested
  • documented
  • changelog entry

@krassowski
Copy link
Member Author

Note: disable pre-commit hook for now as it was destroying code and could not work around it.

@bollwyvl I am prettier illiterate; it is breaking completions.ts (literally does mish-mash) when run and I have no idea how to fix it, nor which piece it does not like. How do I lint/fix this?

@bollwyvl
Copy link
Collaborator

You can disable it explicitly for a single file in .prettierignore. Might be sufficient for this case. There are also some pragmas we can enable, which would let you put it magic comments. But it'd be best if we could fix through normal means, no doubt.

Anyhow, can you post an example of before/after what it's doing as a gist? Maybe we need to upgrade? Please, let's not downgrade, as that would ruin my world on #278, which I'm already scared of fixing the bitrot on.

But indeed, who lints the linters! I disable the hooks, too, because I don't like node doing anything i don't type out with node, yarn or jlpm... git is for me.

version "7.8.3"
resolved "https://registry.yarnpkg.com/@babel/code-frame/-/code-frame-7.8.3.tgz#33e25903d7481181534e12ec0a25f16b6fcf419e"
integrity sha512-a9gxpmdXtZEInkCSHUJDLHZVBgb1QS0jhss4cPP93EW7s+uC5bikET2twEF3KV+7rDblJcmNvTR7VJejqd2C2g==
"@babel/code-frame@^7.0.0", "@babel/code-frame@^7.10.4":
Copy link
Collaborator

Choose a reason for hiding this comment

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

it may well be time to do a full re-solve of all our deps. We should probably do some better sanity pins on, for example, the node-based language servers as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

surprisingly, seems to be largely working :) the most important trick was in krassowski@bf52857

@krassowski krassowski closed this Jul 29, 2020
@krassowski krassowski reopened this Jul 29, 2020
@krassowski
Copy link
Member Author

Also, why didn't Azure CI fire?

@krassowski
Copy link
Member Author

Well, seems that running jlpm prettier from command-line worked better than from a hook...

@krassowski
Copy link
Member Author

All completer tests now pass locally! CI (https://dev.azure.com/krassowskimichal/jupyterlab-lsp/_build) has issues with backend lint:

source activate jupyterlab-lsp && python scripts/lint.py
========================== Starting Command Output ===========================
/bin/bash --noprofile --norc /home/vsts/work/_temp/3d27c3e5-264b-4ba5-8197-7ce2012605f5.sh
/usr/share/miniconda/envs/jupyterlab-lsp/lib/python3.7/site-packages/setuptools/distutils_patch.py:26: UserWarning: Distutils was imported before Setuptools. This usage is discouraged and may exhibit undesirable behaviors or errors. Please use Setuptools' objects directly or at least import Setuptools first.
  "Distutils was imported before Setuptools. This usage is discouraged "
All done! ✨ 🍰 ✨
52 files left unchanged.
py_src/jupyter_lsp/tests/test_paths.py:35: error: Argument 1 to "__call__" of "_SkipifMarkDecorator" has incompatible type "int"; expected "Union[str, bool]"
py_src/jupyter_lsp/tests/test_paths.py:61: error: Argument 1 to "__call__" of "_SkipifMarkDecorator" has incompatible type "int"; expected "Union[str, bool]"
Found 2 errors in 1 file (checked 39 source files)

which seems like a good mypy catch because ~WIN would mean:

>>> ~True
-2
>>> ~False
-1

so we should go for not WIN. Is this correct @bollwyvl?

@bollwyvl
Copy link
Collaborator

Yeah, not WIN looks correct.

@krassowski
Copy link
Member Author

Hooray, azure kicked in after merging master this time!
Not sure if I have yarn lock right, there were conflicts and I just resolved it anew after merging... May be missing something - let's see how it plays out on CI

@krassowski krassowski closed this Aug 7, 2020
@krassowski krassowski reopened this Aug 7, 2020
@bollwyvl
Copy link
Collaborator

bollwyvl commented Aug 7, 2020 via email

@krassowski
Copy link
Member Author

Now the question is why

  • MacOSXThreeSeven and
  • WindowsThreeSeven, WindowsThreeEight

fail while MacOSXThreeSix, WindowsThreeSix, MacOSXThreeEight and all Linux tests pass. I see no pattern...

@krassowski
Copy link
Member Author

Well synced, your message appeared 0.5 second before I pressed enter ;)

@bollwyvl
Copy link
Collaborator

bollwyvl commented Aug 7, 2020 via email

@krassowski krassowski closed this Aug 10, 2020
@krassowski krassowski reopened this Aug 10, 2020
@krassowski
Copy link
Member Author

krassowski commented Aug 10, 2020

Now archspec (whatever this is) also fails to conda install on Windows...

ERROR conda.core.link:_execute(568): An error occurred while installing package 'conda-forge::archspec-0.1.1-pyh9f0ad1d_0'.
CondaError: Cannot link a source that does not exist. C:\Miniconda\Scripts\conda.exe

@bollwyvl
Copy link
Collaborator

On windows: cannot reproduce the archspec thing (not familiar with it), but can consistently reproduce the rect error, so probably will be able to fix it... or at least determine why we should allow it ignoring it.

@@ -108,6 +130,7 @@ Cell Editor Should Equal
Select Completer Suggestion
[Arguments] ${text}
${suggestion} = Set Variable css:.jp-Completer-item[data-value="${text}"]
Wait Until Element Is Visible ${suggestion} timeout=10s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hopefully this works! 🤞

@@ -1,7 +1,7 @@
parameters:
name: Linux
packages: ''
install_cmd: conda install -yn base -c conda-forge conda python-libarchive-c
install_cmd: conda install -yn base conda
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this is an attempt to tackle the archspec issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, the thinking is get something as vanilla as possible, especially for windows, while we get this sorted. who knows what state it's in when they cook the docker images? We could also pin harder, e.g. 4.8 or something.

if we want to start getting far off vanilla, we could investigate installing mamba in this step instead of upgrading conda... no doubt it could shave off some minutes, at the price of sometimes giving different solves. definitely a separate PR, though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

i also need to investigate caching (or packaging) the tectonic stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Was just thinking about mamba while reading this

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll open an issue...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

@bollwyvl bollwyvl left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Auto-complete issue related to file paths
3 participants