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

Remove pointless IO from VFS #523

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Remove pointless IO from VFS #523

merged 1 commit into from
Oct 9, 2023

Conversation

michaelpj
Copy link
Collaborator

Setting up the VFS required IO just so it could capture a reference to a temporary directory that could be used if we wanted to write files out. This is weird and non-compositional: we can just ask the caller where they want the files, and they can make the temporary directory if they want.

Setting up the VFS required IO just so it could capture a reference to a
temporary directory that could be used if we wanted to write files out.
This is weird and non-compositional: we can just ask the caller where
they want the files, and they can make the temporary directory if they
want.
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, nice improvement!

@michaelpj michaelpj merged commit fee511b into master Oct 9, 2023
16 checks passed
@neduard
Copy link

neduard commented Jun 15, 2024

Nice cleanup! Though consider in the future being a bit more careful with functions that are exposed in the API:

  • Consider adding a message in the Changelog for non-backward-compatible changes and include the affected function/constructor names: in this case initVFS was removed and replaced with emptyVFS. This saves maintainers having to git bisect to figure out why software stopped compiling with a new version of stack/cabal : a simple github search on initVFS and they would see the removal - note github search doesn't actually search through PR diffs unfortunately, so this removal doesn't automatically show up.
  • A better way to have done this would have been eg: keep initVFS but add a new emptyVFS and change the code (internals) to use it. This keeps the exposed interface backward compatible. After one or more releases then you can remove initVFS.

In any case, no worries - glad git bisect worked!

@michaelpj
Copy link
Collaborator Author

Hi, that's fair. I should mark this more explicitly but: the lsp packages are definitely still in an "unstable" phase of development. I have quite a lot of work that I still want to do on them, which probably means a lot more major version bumps. I realise that's not ideal, sorry.

@neduard
Copy link

neduard commented Jun 21, 2024

No worries at all! I think as long as people can easily find when there's breaking changes they can always set pin their version and update whenever they wish. Here I think it was a bit difficult to find why compilation broke (though I'm also Haskell beginner so maybe for others it would've been more intuitive)

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