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

Update postinstall message to prevent new user confusion #325

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dmmulroy
Copy link

@dmmulroy dmmulroy commented Mar 15, 2024

The postinstall message is quite confusing for new OCaml devs nowadays who typically by default have ocaml-lsp-server and ocamlformat installed.

This adds some messaging to clear that up.

Copy link

@leostera leostera left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

ocp-indent.opam Outdated Show resolved Hide resolved
Co-authored-by: Nicolás Ojeda Bär <[email protected]>
@dmmulroy dmmulroy requested a review from nojb March 15, 2024 10:56
Copy link
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@lthls
Copy link

lthls commented Mar 15, 2024

Could we go back to why the message is confusing ?
ocp-indent is for indentation, not formatting. It has some overlap with ocamlformat but neither tool replaces the other.
We might need to make it clearer that it won't work in some editors (typically, VScode and its derivatives) but for the traditional editors it works independently of ocaml-lsp-server and ocamlformat.

@nojb
Copy link
Contributor

nojb commented Mar 15, 2024

Could we go back to why the message is confusing ? ocp-indent is for indentation, not formatting. It has some overlap with ocamlformat but neither tool replaces the other.

If you indent your file with ocp-indent then it is no longer formatted according to ocamlformat, so it does not seem possible to use both at the same time. From that perspective, ocp-indent is a lightweight alternative to ocamlformat.

We might need to make it clearer that it won't work in some editors (typically, VScode and its derivatives) but for the traditional editors it works independently of ocaml-lsp-server and ocamlformat.

ocp-indent does work with VS Code: https://marketplace.visualstudio.com/items?itemName=AllanBlanchard.ocp-indent, it is just not part of the recommended setup involving the OCaml LSP.

I think clarifying that ocp-indent and ocamlformat are two alternatives for formatting/indenting your code is good for beginners who haven't necessarily been following in detail the evolution of the OCaml ecosystem.

@lthls
Copy link

lthls commented Mar 15, 2024

If you indent your file with ocp-indent then it is no longer formatted according to ocamlformat, so it does not seem possible to use both at the same time. From that perspective, ocp-indent is a lightweight alternative to ocamlformat.

ocamlformat has support for reading .ocp-indent configuration files, so it is possible to use both at the same time, most of the time. Typically, ocp-indent while editing code and ocamlformat before committing.

ocp-indent does work with VS Code: https://marketplace.visualstudio.com/items?itemName=AllanBlanchard.ocp-indent, it is just not part of the recommended setup involving the OCaml LSP.

I wasn't aware of that. The user-setup package will not take care of setting it up, so maybe something could be said here.

I think clarifying that ocp-indent and ocamlformat are two alternatives for formatting/indenting your code is good for beginners who haven't necessarily been following in detail the evolution of the OCaml ecosystem.

I don't think so, it's quite clear that one handles indentation while the other handles formatting. You can do your indentation with a formatter if you want, or your formatting with an indenter, but I don't see the point in making people think they don't need both of them when they have different and complementary purposes.

@nojb
Copy link
Contributor

nojb commented Mar 15, 2024

ocamlformat has support for reading .ocp-indent configuration files, so it is possible to use both at the same time, most of the time. Typically, ocp-indent while editing code and ocamlformat before committing.

Is this an actual workflow that you use? My understanding is that ocamlformat can read .ocp-indent but its result is not always (or even often) ocp-indent-invariant. But I am not an ocamlformat user, so perhaps things have improved in the past.

but I don't see the point in making people think they don't need both of them when they have different and complementary purposes.

I don't have a horse in this race (I am a happy ocp-indent user), but it strikes me as odd to recommend that users install and use both an auto-indenter and an auto-formatter... but like I said, I don't have a horse in this race, so I let you decide the way forward.

Cheers!

@dmmulroy
Copy link
Author

I've been helping teach a bunch of new folks OCaml over the past few months, and without fail every single person (especially those using Vim or Neovim), get caught up on the postinstall message from ocp-indent when installing ocamlformat.

Installing ocamlformat is the second step issued to newcomers to OCaml on the ocaml.org site:
image

Which then causes this output to be one of the first messages shown to newcomers in the ecoystem:

image

I think this is incredibly confusing and my actual preference would be to remove the postinstall script altogether. But, I think at the very last indicating that newcomers don't need to take action if they're using ocamlformat is a fine solution as well.

@lthls
Copy link

lthls commented Mar 15, 2024

Ah, so the issue is that this message is shown to people who have not explicitly asked for ocp-indent to be installed.
ocamlformat depends on ocp-indent not for the binary, but for the library (to parse .ocp-indent files). So one solution could be to split ocp-indent into two packages. Another solution might be to see with the opam people if it makes sense to not display post-installation messages for packages that were not required explicitly on the command-line.
But it doesn't make sense to remove this message: a lot of people still install ocp-indent explicitly, and the instructions are useful for them.

The message isn't displayed if you also install opam-user-setup, by the way. Maybe adding that to the list of packages for initial installation would solve the problem ?

@dbuenzli
Copy link
Contributor

You can use filters on messsages. So you could perhaps show the message only if ocamlformat is not installed with something like {!ocamlformat:installed}.

@lthls
Copy link

lthls commented Mar 15, 2024

Is this an actual workflow that you use? My understanding is that ocamlformat can read .ocp-indent but its result is not always (or even often) ocp-indent-invariant.

Yes, I use ocp-indent when editing and ocamlformat before pushing commits (when I work on repos that have a mandatory formatting policy). I don't need both tools to produce the same output, it's fine if ocamlformat messes with the indentation (well, it's annoying, but if it's part of the repo's policy it's unavoidable).

I don't have a horse in this race (I am a happy ocp-indent user), but it strikes me as odd to recommend that users install and use both an auto-indenter and an auto-formatter...

My supposition is that ocp-indent gets invoked very often, typically on every press of the tab key, to indent some current context. Formatting, on the other hand, is something you usually do less regularly, for example on save or on commit.
This is reflected in ocp-indent and ocamlformat's features: the former can handle pieces of code with little context, sometimes not even syntactically correct, while the latter requires your whole file to be parsed correctly but can handle things like inserting line breaks that are out of scope for ocp-indent.

@AltGr
Copy link
Member

AltGr commented Mar 19, 2024

I like @dbuenzli's suggestion best: it's approximative, but should avoid the confusion described here. Ideally we could write {ocp-indent:requested} but opam doesn't provide this invormation. The corner-case would be someone trying to install ocp-indent manually, after having installed ocamlformat...

Or just a rewording, as initially proposed. maybe just starting the message with If you want to use ocp-indent from within Vim or Emacs, [...] would help to recognise when you're not the target of the message.

As for the usage: indeed ocp-indent is mostly useful as an inline tool, while ocamlformat is focused on whole-file processing. I am curious how the base VScode+LSP experience does the indentation (eg when on an empty line), by the way...

@dmmulroy
Copy link
Author

dmmulroy commented Apr 3, 2024

I've updated the message to:

  "If you want to use `ocp-indent` from within Emacs or Vim directly, It requires additional configuration for use in editors. Install package 'user-setup', or manually:

* for Emacs, add these lines to ~/.emacs:
  (add-to-list 'load-path \"%{share}%/emacs/site-lisp\")
  (require 'ocp-indent)

* for Vim, add this line to ~/.vimrc:
  set rtp^=\"%{share}%/ocp-indent/vim\"

* If you are seeing this message as a result of installing `ocamlformat`, you may ignore the above instructions and continue using your existing LSP and formatting configuration. ```

@dmmulroy dmmulroy requested review from nojb and leostera April 3, 2024 12:18
@nojb
Copy link
Contributor

nojb commented Apr 4, 2024

@dmmulroy I think you should get reviews from the maintainers of this repository (eg @AltGr or @lthls) rather than from myself. @dbuenzli has suggested something above that seems to convince @AltGr; you may want to look into it. Cheers!

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.

6 participants