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

Locate doc improvements #1562

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Jan 20, 2023

This fixes #1561.

Before iterating on a cmt's Typedtree we tried to rebuild the environment from the summary.
In the case of Melange this fails with the error: Cannot find module Bs_stdlib_mini

In am not sure of the source of that error (maybe a missing installation artifact ?).
Ignoring it still enable Merlin to find the doc in the typedtree in the present case and if not the code would fallback to the old method of getting the comments (by using the comments list in the cmt).

I took this opportunity to start doing some refactoring in the last commit.

@voodoos voodoos requested a review from trefis February 1, 2023 12:21
@voodoos
Copy link
Collaborator Author

voodoos commented Feb 1, 2023

The PR mixes bug fixes and refactoring, it is probably best to review commit by commit.

@voodoos voodoos added this to the Next release milestone Feb 1, 2023
Copy link
Contributor

@trefis trefis left a comment

Choose a reason for hiding this comment

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

The 1st commit seems weird to me.

I wouldn't expect env_of_only_summary to fail (could there be a version mismatch between merlin and ocaml… that is somehow not caught when checking magic numbers?).
But if it does, I don't think silently ignoring the error and falling back to a degraded behavior is the right way to go about it.
I'd rather give a proper error the user (potentially while still trying to produce a "best effort" answer at the same time).

The next two commits LGTM. (The changelog will have to wait until the dust settles on this PR).

@voodoos voodoos marked this pull request as draft February 23, 2023 09:27
@voodoos voodoos removed this from the Next release milestone Feb 23, 2023
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.

Ocaml_typing.Envaux.Error when hovering over some functions
2 participants