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

ocaml: prepare for filetype separation, drop ols support #3732

Merged
merged 1 commit into from
May 27, 2021

Conversation

undu
Copy link
Contributor

@undu undu commented May 22, 2021

The ocaml filetype is currently used for several, different file
formats. This causes problems as not all tools support all formats.
New filetypes are introduced to support this separation, this needs some
changes in ale that are fortunately backwards-compatible.

These change add ocamlinterface file support for ocp-indent, merlin,
ocamlformat and ocaml-lsp. For ocaml-lsp I took the liberty to
add all recognised language ids, even if they are not supported.

ols has been removed as the project has been abandoned since 2019,
users are better served by other tools.

The PR for splitting the filetypes is at ocaml/vim-ocaml#61

The ocaml filetype is currently used for several, different file
formats. This causes problems as not all tools support all formats.
New filetypes are introduced to support this separation, this needs some
changes in ale that are fortunately backwards-compatible.

These change add ocamlinterface file support for ocp-indent, merlin,
ocamlformat and ocaml-lsp. For ocaml-lsp I took the liberty to
add all recognised language ids, even if they are not supported.

ols has not been changed as the project has been abandoned since 2019.
@undu
Copy link
Contributor Author

undu commented May 22, 2021

appveyor failure is unrelated to the change:

git clone -q --depth=10 https://github.com/dense-analysis/ale.git C:\testplugin
fatal: unable to access 'https://github.com/dense-analysis/ale.git/': Could not resolve host: github.com
Command exited with code 128

Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Changes seem ok except for removing ols that is a potential breaking change. Currently there is no defined procedure to remove supported tools from ALE and I would refrain from doing so unless is first announced (somehow) to give users time to phase the tool out or request it not to be removed.

I recommend moving the changes to remove ols support to a separate PR with an associated issue to inform and start discussion on if is ok to remove it from ALE.

@undu
Copy link
Contributor Author

undu commented May 25, 2021

That makes sense, I'll remove the changes to remove ols.

@undu undu force-pushed the ocaml_interface branch from 380645b to 59dae8a Compare May 25, 2021 19:19
@undu undu requested a review from hsanson May 25, 2021 19:20
Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Looks good to me.

@hsanson hsanson merged commit a02a4f2 into dense-analysis:master May 27, 2021
@undu undu deleted the ocaml_interface branch May 27, 2021 07:44
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