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

Desugar local build tools to build-tool-depends (fixes #516) #567

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sol
Copy link
Owner

@sol sol commented Sep 19, 2023

@sol sol marked this pull request as ready for review September 19, 2023 12:37
@sol sol requested a review from mpilgrem September 19, 2023 12:38
Copy link
Collaborator

@mpilgrem mpilgrem left a comment

Choose a reason for hiding this comment

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

Looks good to me, but does Hpack.Config.determineCabalVersion need to be amended if build-tool-depends in a Cabal file implies, at least, cabal-version: 2.0?

@@ -351,7 +351,6 @@ data RenderBuildTool = BuildTools String | BuildToolDepends String

renderBuildTool :: (BuildTool, DependencyVersion) -> RenderBuildTool
renderBuildTool (buildTool, renderVersion -> version) = case buildTool of
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume it is a stylistic choice, but I noted the continued use of case even though type BuildTool has only one constructor.

@sol sol added the blocked label Sep 20, 2023
@sol
Copy link
Owner Author

sol commented Sep 20, 2023

@mpilgrem thanks a lot for the review. This is not going to land for now. I updated the PR description.

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

Successfully merging this pull request may close these issues.

None yet

2 participants