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

Fish completions #105

Closed
thomsj opened this issue May 30, 2019 · 29 comments · Fixed by #235
Closed

Fish completions #105

thomsj opened this issue May 30, 2019 · 29 comments · Fixed by #235
Labels
fish shell help wanted Extra attention is needed

Comments

@thomsj
Copy link
Contributor

thomsj commented May 30, 2019

Are there any plans for adding fish completions? Perhaps this could be achieved with a man page, as it would probably be better than having third-party completions from, for example, an Oh My Fish plugin. Maybe the installation script could add completions to ~/.config/fish/completions, as this could allow for completions similar to plugin-nvm.

@Schniz
Copy link
Owner

Schniz commented May 31, 2019

Hi, I don't use fish, but I'll be more than happy to get a PR with this 😄
If we can test it and make sure that new features will have to add completions - it would be even awesome...er?

😄

@Schniz Schniz added the help wanted Extra attention is needed label May 31, 2019
@thomsj
Copy link
Contributor Author

thomsj commented May 31, 2019

I'll try to implement this in the next week or so. 🙂

@thomsj
Copy link
Contributor Author

thomsj commented Jun 23, 2019

Hi @Schniz,

I've done the first part of this, the completions themselves. If you want to try the completions you can find the script here. I'll hopefully see about adding tests soon.

I noticed a few things (mostly documentation stuff) when I was using fnm / writing completions, do you want me to create a few separate issues?

@Schniz
Copy link
Owner

Schniz commented Jun 24, 2019

If you want to try the completions you can find the script here.

Wow! 💰 This is some hardcore stuff! do you use something to generate it? Can it be a script we run on every release?

I noticed a few things (mostly documentation stuff) when I was using fnm / writing completions, do you want me to create a few separate issues?

I'd really appreciate it!

@thomsj
Copy link
Contributor Author

thomsj commented Jun 25, 2019

Thanks. 🙂

I just wrote the completions manually, but I think it should be possible to use a script. My initial idea for how to test that new features have completions was to parse the output of the help pages, but I think this approach could be used to write the completions instead, if a few changes were made to the help pages.

For example, if the synopsis of fnm use --help was changed to something like, fnm use [OPTION]... [ALIAS | VERSION], it would be possible to detect when to complete with versions, aliases or both.

@thomsj
Copy link
Contributor Author

thomsj commented Jun 26, 2019

Would it be possible to write the script in ReasonML and use either FnmApp or the other executable modules, instead of parsing the help pages?

@Schniz
Copy link
Owner

Schniz commented Jul 1, 2019

Would it be possible to write the script in ReasonML and use either FnmApp or the other executable modules, instead of parsing the help pages?

that would make more sense IMO! At least, parsing Cmdliner's data structures, that we use in FnmApp. Unfortunately, this isn't a part of Cmdliner at this time but we might write something nice ourselves!

@thomsj
Copy link
Contributor Author

thomsj commented Jul 21, 2019

Sorry I've taken ages to get back to you, @Schniz.

I haven't used Reason before, but I'd like to see if I can come up with something for this, so I was wondering if you could tell me what editor plugin you use for fnm/Reason. Is it one of these?

@Schniz
Copy link
Owner

Schniz commented Jul 22, 2019

Sorry I've taken ages to get back to you

You have been missed 😜

Is it one of these?

Indeed!

I use @jordwalke's https://github.com/jordwalke/vim-reasonml since I use vim and it works exactly how I expect it to work! (fast and reliable)
I also tried @jaredly's Reason Language Server for VSCode and it was very very good IIRC, but VSCode is acting weird for me so I stopped trying using it 😄

but I'd like to see if I can come up with something for this

Let me know if you need any help! Maybe we can have a different representation for the CLI options, that we can later use with Cmdliner as a frontend that consumes it — and the fish completions as a frontend (and zsh/bash completions too eventually 😈!)

@thomsj
Copy link
Contributor Author

thomsj commented Jul 23, 2019

You have been missed 😜

😂

Thanks for the info on the plugins, I'll give them a try. 😀

Maybe we can have a different representation for the CLI options, that we can later use with Cmdliner as a frontend that consumes it — and the fish completions as a frontend (and zsh/bash completions too eventually 😈!)

Nice idea! 😃

Let me know if you need any help!

Hopefully I'll be able to start playing with this/Reason this week. I'll let you know how I get on. 😊

@thomsj
Copy link
Contributor Author

thomsj commented Aug 1, 2019

Hi @Schniz,

I just tried Reason Language Server for VS Code. Unfortunately, it displays a lot of errors, such as Error: Unbound module Fnm, even though the build succeeds. This is with the master branch before I have changed anything.

Is this what you experienced, and why you stopped using VS Code?

I tried the extension with an example esy project and it seemed to work fine.

@thomsj
Copy link
Contributor Author

thomsj commented Aug 5, 2019

I asked on the ReasonML Discord and I was told that it was because esy.buildsInSource was set to "_build" in package.json. I changed it to false (the default) and built again, and after that the extension works. 😀

@Schniz
Copy link
Owner

Schniz commented Aug 5, 2019

Woah! Sorry for not responding. I forgot to answer! I wonder why it doesn’t work with buildsInSources, I use it all the time. Maybe i shouldn’t? I’ll try to figure out on Discord

@thomsj
Copy link
Contributor Author

thomsj commented Aug 5, 2019

That's okay. 😊

Since I don't know Reason I can't say for sure, but I think it's because when using dune with esy it always looks in $cur__target_dir and never in _build:

https://github.com/jaredly/reason-language-server/blob/fb1c1a030e5e7ab705f82f42dd5c31f1591cbd48/src/analyze/BuildSystem.re#L191-L202

If you want to see what was discussed on Discord, here is a link to the first message I posted (in the editorsupport channel).

@Schniz
Copy link
Owner

Schniz commented Aug 7, 2019

Interesting. Thanks for sharing. I think we should have a PR that removes buildsInSources then, so everyone can contribute.

@thomsj
Copy link
Contributor Author

thomsj commented Aug 9, 2019

Is there a particular reason why you use "buildsInSource": "_build"? Is it because it's what the docs say to use if you're using dune, or another reason?

We could also submit a pull request to fix the language server, if you want to continue using _build?

@Schniz
Copy link
Owner

Schniz commented Aug 11, 2019

Is there a particular reason why you use "buildsInSource": "_build"?

Because using _esy/default/build/default/executable/FnmApp.exe felt like I'm dealing with internal _esy stuff. Maybe it isn't really the case. Seems that replacing _build with _esy/default/build can work, and make fnm follow esy's conventions :)

@thomsj
Copy link
Contributor Author

thomsj commented Sep 28, 2019

I haven't had time to start this yet. 🤦‍♂️

I read all the ReasonML docs and started working on a small fix for fnm ages ago, but then got sidetracked. Hopefully I'll have time to work on this in the next few weeks.

A few weeks ago I noticed that QPDF has options for the binary to help enable completions. How would you feel about doing this or adding a command to fnm for completions, instead of having a separate script?

I was thinking we could maybe have something like:

fnm print-completions --bash

or:

fnm print-completions --shell=bash

For people using fish, for example, they could then run:

fnm print-completions --fish > ~/.config/fish/completions/fnm.fish

to save the completions.

@Schniz
Copy link
Owner

Schniz commented Sep 28, 2019

Hehe no worries! I think since we have the fnm env —shell=... command we can have it built into the env command instead of a different one.

I merged #142 so it’d be easier to contribute :)

@thomsj
Copy link
Contributor Author

thomsj commented Sep 29, 2019

Thanks for merging #142. 😊

Do you mean add a new option to env or use the existing --shell option?

@Schniz
Copy link
Owner

Schniz commented Sep 29, 2019

The existing env command adds stuff to the shell. It adds env vars by default, and optionally adds hooks to directory change. It can also add completions optionally, and we can make it the default if it works well!

@thomsj
Copy link
Contributor Author

thomsj commented Sep 29, 2019

If I was using it with fish I would prefer to just run the command once, to get the completions, and save the output to ~/.config/fish/completions/fnm.fish.

I've never used completions with bash or zsh so I don't know if they can be configured to source completions only when required.

@thomsj
Copy link
Contributor Author

thomsj commented Sep 29, 2019

Do you use refmt for the fnm source code?

@Schniz
Copy link
Owner

Schniz commented Oct 1, 2019

If I was using it with fish I would prefer to just run the command once, to get the completions, and save the output to ~/.config/fish/completions/fnm.fish.

Sounds good. I don't have any prior knowledge regarding completions so I'll take your word for it 😄

Do you use refmt for the fnm source code?

Yeah, running esy fmt should format everything

@thomsj
Copy link
Contributor Author

thomsj commented Oct 6, 2019

What version of refmt do you have installed? I have 3.5.0 and it changes quite a few things. In particular, it removes the space before the bracket after try%lwt.

For example:

try%lwt (iterate()) {

becomes:

try%lwt(iterate()) {

@Schniz
Copy link
Owner

Schniz commented Oct 8, 2019

Yeah, I did a refmt in #146. Now everything is good. I need to break the build if it's not good.

@thomsj
Copy link
Contributor Author

thomsj commented Oct 8, 2019

How do you break the build? Delete all the esy stuff?

Schniz added a commit that referenced this issue Oct 8, 2019
This PR adds a GitHub action (🎉) that checks for the latest refmt syntax.

This is done thanks to @thomsj which brought up the problems with formatting in the repo! 👏 #105 (comment)

The current implementation is not ideal: it installs esy and then installs OCaml and @esy-ocaml/reason. A better way to do it is to distribute refmt as a standalone binary from the latest Reason release. This will be fast to install and therefore fast to integrate with CI.

An approach to do so is to add an artifact to the CircleCI runs, and then use something like circleci-artifacts.now.sh to fetch the artifact, extract it and profit 💰

In the meantime, it takes <4m to run it, which is much faster than the Azure Linux build, not to mention the Windows one.
@Schniz
Copy link
Owner

Schniz commented Oct 25, 2020

image

Shell completions are generated using the binary now 😄

@thomsj
Copy link
Contributor Author

thomsj commented Oct 25, 2020

I'm looking forward to trying the Rust version. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fish shell help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants