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

Tweak manifest plugin to return ELP compatible information. #2910

Merged
merged 3 commits into from
Aug 11, 2024

Conversation

robertoaloi
Copy link
Contributor

@robertoaloi robertoaloi commented Aug 7, 2024

Starting the discussion around the actual format to use in the manifest plugin.

For the time being, instead of returning the raw context, I'm basically adopting the same format previously adopted by the build_info plugin, used by the ELP language server. Using the same format avoids changes to ELP itself.

I am also adding a json format to make it easier for tools to consume the information.

Test Plan

Generate build information for the Erlang LS repo via ELP (internally using the build_info plugin):

elp build-info --project ~/git/github/erlang-ls/erlang_ls --to /tmp/erlang-ls-build

Locally modify ELP to use the experimental manifest plugin instead, then:

elp build-info --project ~/git/github/erlang-ls/erlang_ls --to /tmp/erlang-ls-manifest

Finally compare the two generated files:

diff /tmp/erlang-ls-build /tmp/erlang-ls-manifest

The only difference, for each application, is:

<         "include"
---
>         "include",
>         "src",
>         ""

This should not be an issue, but we can double check that as a follow-up.

@ferd
Copy link
Collaborator

ferd commented Aug 7, 2024

Since this provider was marked as experimental I don't see big problems with changing the format.

That none of the tests captured it does point to some of these tests eventually needing some hardening, particularly to get out of experimental mode.

@robertoaloi
Copy link
Contributor Author

robertoaloi commented Aug 8, 2024

That's exactly the plan. One thing we're considering is if we shouldn't simply output JSON, easier for tools to consume, given that OTP 27 comes with it. Of course we'd have to detect whether the json module is available and bail out if that's not the case.

@tsloughter
Copy link
Collaborator

Yes, should output json I think. Or at least take a -o json argument.

Comment on lines 77 to 78
format_error(no_json_module) ->
io_lib:format("The 'json' module is not available. Either upgrade to OTP 27 or use a different format.", []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Either upgrade to OTP 27 or newer, or select a different output format." maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the spelling, thanks.

Copy link
Collaborator

@ferd ferd left a comment

Choose a reason for hiding this comment

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

Seems good to me at this point, anything else you wanted on this or it's ready to go?

@robertoaloi
Copy link
Contributor Author

Ready to go for now.

@ferd ferd merged commit 1959c16 into erlang:main Aug 11, 2024
6 checks passed
@robertoaloi
Copy link
Contributor Author

@ferd Not terribly urgent, but what's the ETA for the next rebar3 release?

@ferd
Copy link
Collaborator

ferd commented Aug 20, 2024

Yeah I'm probably due to cut a new one. Will try it this week, next week at the latest.

@ferd
Copy link
Collaborator

ferd commented Aug 29, 2024

3.24.0 was just released: https://github.com/erlang/rebar3/releases/tag/3.24.0

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.

3 participants