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

Keep track of not loaded files for cabal #453

Merged
merged 25 commits into from
Mar 2, 2025
Merged

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented Feb 25, 2025

The main use case:
When we fail to load some files $X$, we know exctaly what files $Y$ we actually did try to load. Where $Y \in X$.

$Y>1$ only make sense if we are using cabal's multiple home unit repl features.

We need this to reduce the number of files we put into senquential loading loop once batch load is failed for haskell/haskell-language-server#4445

TODO:

  • The logic is simple, but I think we should add a test for case $Y > 1$. But I don't know how to trigger such loading errors.

@fendor fendor self-requested a review February 25, 2025 09:57
@soulomoon
Copy link
Collaborator Author

soulomoon commented Feb 25, 2025

The basic idea is that we can include new files to be loaded along with old files into LoadWithContext.
If we fail to load them togather, emit the CradleError with the actual files we did call cabal with.
Tests failing-cabal-multi-repl-with-shrink-error-files added, now the patch ready to be reviewed @fendor

Copy link
Collaborator

@fendor fendor 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 PR!
I have a couple of comments and improvements, but otherwise I think this is good to go!

@soulomoon soulomoon requested a review from fendor February 26, 2025 21:08
@soulomoon soulomoon changed the title Keep track of unloaded files for cabal Keep track of not loaded files for cabal Feb 27, 2025
@soulomoon
Copy link
Collaborator Author

cc @fendor , I've addressed the comments, please take a look

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! One nitpick comment, once you have decided whether you agree or not, you should be able to press the merge button :)

@fendor fendor merged commit bc502c9 into haskell:master Mar 2, 2025
22 checks passed
@soulomoon soulomoon mentioned this pull request Mar 2, 2025
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