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 texlab for LaTeX #288

Merged
merged 33 commits into from
Jul 28, 2020
Merged

Conversation

bollwyvl
Copy link
Collaborator

@bollwyvl bollwyvl commented Jun 30, 2020

Binder

References

Code changes

  • add texlab to test/demo environment
    • also tectonic
    • also chktex
  • add texlab spec
    • add to setup.cfg
    • add to unit tests
  • add to docs
    • i've put it under a new section...
      • i think we need to make this one as good as we can, but there will be rough edges for now, like completion
  • fix sending null instead of {} for initializedParams
    • unsurprisingly, rust is really hard core about types 🤣
  • add robot tests
    • editor
      • rename
      • jump
      • diagnostic
        • it's sending back messages, but they're always empty... need to investigate
        • chktex is the engine of choice referenced in the documentation... apparently there are some inconsistencies in the one that comes with different platform distributions.

        investigating the feasibility of building/packaging this

    • config
      • by default, it uses latexmk which is hard to ensure under test. a config test of using tectonic would be cool
  • figure out how to get RTD to pick up new spec

Not Appearing on this issue

User-facing changes

  • Can now view get some feedback on .tex files 🎉

Backwards-incompatible changes

  • None

Chores

  • linted
  • tested
  • documented
  • changelog entry

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jul 2, 2020 via email

@bollwyvl
Copy link
Collaborator Author

I did get chktex built, so will be able to push forward with this, soon. If nothing else, it will find if my potentially-wonky OSX builds actually work.

@bollwyvl
Copy link
Collaborator Author

it's looking good:
Screenshot from 2020-07-22 21-53-58

@bollwyvl
Copy link
Collaborator Author

One issue, though, is that the diagnostics don't show up on save... and apparently should by default. Will add a test that enables them, i guess.

@bollwyvl bollwyvl changed the title [wip] Add texlab for LaTeX Add texlab for LaTeX Jul 25, 2020
@bollwyvl bollwyvl requested a review from krassowski July 25, 2020 17:54
@krassowski
Copy link
Member

Re-diagnostics - they work in a new notebook:

Screenshot from 2020-07-25 22-40-43

@krassowski
Copy link
Member

They even work after copy-pasting all cells to the new notebook:
Screenshot from 2020-07-25 22-43-15

But markdown stopped working for me here...

@krassowski
Copy link
Member

It works even after closing the notebook and opening it again. It consistently does not work in the Foreign extractors.ipynb notebook. Could it have to do with the space in the file name?

@krassowski
Copy link
Member

It seems that it does. Renaming Untilted.ipynbUntil ted.ipynb broke them. This is good to know and we should test it - it is likely that users will have filenames with spaces.

"#### Example: Getting a $\\LaTeX$ stack\n",
"\n",
"```bash\n",
"conda install -y conda-forge tectonic texlab chktex\n",
Copy link
Member

Choose a reason for hiding this comment

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

Is this conda-only or is there a reasonable (maybe not single command I guess) alternative for let's say Ubuntu users?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

latex installation, care, and feeding, is one of my personal circles of support hell. for example, because i run a small root partition, i can't even use the "easy" route on my machine, unless i handpick all of the stuff it would need to work, because i would run out of disk space.

with this, like nodejs, or anything else in the stack, providing inaccurate install docs is worse than nothing, so i think the best thing we can do is:

  • provide a link to the various official install docs
  • show an example of what we know works, because keeping it working is on the critical path to us merging adding/changing feature.

the only way i'd be interested in documenting or supporting n distros, would be if we had additional Dockerfiles we ran the full battery of tests against with the release tarballs... and that seems like an awful lot for this

That being said, it looks like the docs aren't pick up the new spec:

https://jupyterlab-lsp--288.org.readthedocs.build/en/288/Language%20Servers.html#Other-Scientific-Languages

Copy link
Member

Choose a reason for hiding this comment

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

About the docs - R does not show up on the PR builds too, but it is ok on master; would it have to do with your changes, or is it a limitation of the PR builds?

Copy link
Member

Choose a reason for hiding this comment

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

Well, my personal standpoint is that having a Debian/Ubuntu install is usually good enough. Not only because it is a common one, but also because I can always google "how to install X ubuntu package equivalent on Y" and it more often works than it doesn't (unless speaking of exotic distros). This trick also works for Y=Mac (sometimes).

But I think this is not holding this PR from being merged - let's just make sure we show links to official docs of the tools we require to make latex work!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess let's see how the docs build goes... I didn't have to do anything too fancy to the specs.

Debian/Ubuntu install is usually good enough

I suppose... but again, I've never tried to do one... it's definitely one of those things I Docker-and-forget in a build toolchahin. Indeed let's hope that the texlab docs link is sufficient.

@krassowski
Copy link
Member

and we should test it

come to think of it, this is why Foreign extractors.ipynb has space in the name in the first place. I think we discussed this earlier - good job past us.

@bollwyvl
Copy link
Collaborator Author

Excellent catch on the spaces. Of course that would break something in a stack this weird. Everything can go wrong once humans who, you know, use computers to get work done, start getting to make choices! Sigh.

I know there are some file path normalization things that changed in 2.2.0, and we likely have some more places where bad stuff happens. I'll do a little more looking later today...

@bollwyvl
Copy link
Collaborator Author

So in handleDiagnostic, the response coming back is not URL-escaped, while our internal reference in document_info is.

Screenshot from 2020-07-26 14-53-34

I guess we need to figure The One Way we are going to standardize these... I can start looking on this pr, or we can handle it on a separate issue. The worst way, we'll want a util.isSameUri. The better way would be to use the upstream https://github.com/microsoft/vscode-uri which has no dependencies (:tada:) and is probably what enables all this inconsistent wire behavior to actually work in The Client.

@krassowski
Copy link
Member

Great find, thank you! I think, it's best to address it here so we can have test in. I would be happy with either of the two (an util or the vscode-uri way)!

@bollwyvl
Copy link
Collaborator Author

I tried out vscode-uri, and one of the first things it did was error out with a max recursion depth issue. yeesh. have rolled that back, and will start over. we do already have a util for checking uri equivalence, i guess the right play will be to use that more, and make it more robust to weirdness as we observe it.

@bollwyvl
Copy link
Collaborator Author

the OSX fail looks spurious, checking out the win fails on my vm...

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jul 27, 2020 via email

@bollwyvl
Copy link
Collaborator Author

Back to green, no further changes intended, and might even be out of the loop for a couple days (wifi-depending).

@krassowski krassowski merged commit acf17ab into jupyter-lsp:master Jul 28, 2020
@bollwyvl
Copy link
Collaborator Author

Hooray!

Also shows up fine on the docs.

This was referenced Sep 6, 2020
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