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

Refactor Rebuild monad into writer over sets #9843

Closed
wants to merge 2 commits into from

Conversation

edmundnoble
Copy link
Contributor

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

I noticed that this seemed like a more natural way for the Rebuild monad to work. Perhaps this even saves us some file watching, I didn't look into it that deeply.

-> [MonitorFilePath]
-> IO MonitorStateFileSet
go !singlePaths !globPaths [] =
return (MonitorStateFileSet (reverse singlePaths) (reverse globPaths))
return (MonitorStateFileSet singlePaths globPaths)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This worries me: reverse implies the ordering is significant. (I know it's reversing because it uses : to prepend paths, but that still means the original path order is considered important enough to reconstruct afterward.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this worried me a bit too. I figure this was just out of habit, but I might be wrong.

@Mikolaj
Copy link
Member

Mikolaj commented May 1, 2024

What's the status of this one (note that #9844 depends on it)? If a design discussion would be prudent before merging one or the other PR, please don't hesitate to open an issue ticket and mark it with the discussion label and lay out the design questions.

@Kleidukos
Copy link
Member

@edmundnoble Humble ping to get your attention on this. :)

@edmundnoble
Copy link
Contributor Author

I will just close this. I'm not sure why I thought #9844 depended on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants