Skip to content

Conversation

@liam923
Copy link
Contributor

@liam923 liam923 commented Jun 30, 2025

This is a duplicate of #152 that I made to get CI to run.

@liam923 liam923 force-pushed the merlin-downstreams-for-renaming-voodoos branch 2 times, most recently from e1f10a1 to 0209b80 Compare July 1, 2025 20:40
@voodoos
Copy link
Contributor

voodoos commented Jul 3, 2025

Thanks for the fixes @liam923 the diff also looks ok to me.

@liam923 liam923 force-pushed the merlin-downstreams-for-renaming-voodoos branch from 0209b80 to 740f5f5 Compare July 10, 2025 20:08
@liam923
Copy link
Contributor Author

liam923 commented Jul 11, 2025

@voodoos and I have collectively reviewed up to 740f5f5. The most recent 7 commits are changes I made while reviewing and should get a second set of eyes. Here's a commit-by-commit commentary on those 7 commits:

  • 0eed693: Bring locate.ml slightly more in line with the upstream version, as well as a minor stylistic change. (The CR is addressed in a later commit.)
  • 8235236: Some changes to get more in line with upstream.
  • a14182c: Small change that is specific to oxcaml Merlin
  • b59817c: Some stylistic changes and comment edits in oxcaml-specific Merlin code
  • 977cfca: Make a test work with oxcaml. Tests in oxcaml Merlin are unable to use dune, so this commit changes a test to avoid using dune.
  • 70801df: Changes to a test to bring it in line with upstream.
  • d69ecf1: Resolve the aforementioned CR.

I've also verified that this version of Merlin has expected test output in our internal codebase.

@liam923
Copy link
Contributor Author

liam923 commented Jul 11, 2025

I'm not sure why CI is failing - tests are succeeding for me locally. I'll look into it.

@liam923
Copy link
Contributor Author

liam923 commented Jul 16, 2025

I'm not sure why CI is failing - tests are succeeding for me locally. I'll look into it.

Turns out a .merlin file that I added for a test was getting ignored by get, so it existed locally but not in the repo. The issue should now be resolved.

@dkalinichenko-js dkalinichenko-js self-requested a review July 16, 2025 15:27
Copy link
Contributor

@dkalinichenko-js dkalinichenko-js left a comment

Choose a reason for hiding this comment

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

I reviewed the last 7 commits and am happy with them.

@liam923 liam923 merged commit f800beb into main Jul 16, 2025
2 checks passed
@liam923 liam923 deleted the merlin-downstreams-for-renaming-voodoos branch July 16, 2025 15:29
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.

4 participants