Draft
Conversation
Collaborator
Author
|
The PR mixes bug fixes and refactoring, it is probably best to review commit by commit. |
trefis
requested changes
Feb 22, 2023
Contributor
There was a problem hiding this comment.
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_miniIn 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.