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

haddock-project: add CommonSetupFlags #10393

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

Conversation

9999years
Copy link
Collaborator

@9999years 9999years commented Sep 27, 2024

Split off of #10292. This lets cabal haddock-project use the standard CommonFlags that other commands use to set verbosity and so on.

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

@9999years 9999years force-pushed the wiggles/haddock-project-parse-common-flags branch from f2afeea to 89cb6a8 Compare September 27, 2024 23:35
@9999years 9999years changed the title haddock-project: add CommonFlags haddock-project: add CommonSetupFlags Sep 27, 2024
Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

Note: API change, can't be backported.

@9999years 9999years force-pushed the wiggles/haddock-project-parse-common-flags branch from 89cb6a8 to 7fa0e5e Compare September 30, 2024 16:55
@9999years 9999years mentioned this pull request Sep 30, 2024
2 tasks
@9999years 9999years force-pushed the wiggles/haddock-project-parse-common-flags branch from 7fa0e5e to 14510a9 Compare September 30, 2024 17:51
@9999years 9999years marked this pull request as ready for review September 30, 2024 18:40
@9999years 9999years added the merge me Tell Mergify Bot to merge label Sep 30, 2024
@@ -1408,7 +1408,8 @@ legacySharedConfigFieldDescrs constraintSrc =
configPackageDBs
(\v conf -> conf{configPackageDBs = v})
]
. filterFields (["verbose", "builddir"] ++ map optionName installDirsOptions)
. aliasField "keep-temp-files" "haddock-keep-temp-files"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this line doing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, this line shouldn't be here. It's so that when the cabal.project parser sees haddock-keep-temp-files, it uses the new keep-temp-files field, but I don't actually add that until #10292.

@@ -457,7 +458,8 @@ data HaddockProjectFlags = HaddockProjectFlags
defaultHaddockProjectFlags :: HaddockProjectFlags
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are haddock-project options defined in Cabal library at all.. I understand nothing to do with you but it's a cabal-install command.

@mpickering
Copy link
Collaborator

I am not sure this is the right direction of travel -- why are we propagating the design flaw of reusing the options parsers for the ./Setup interface for the cabal-install interface?

@9999years 9999years force-pushed the wiggles/haddock-project-parse-common-flags branch from 14510a9 to 5730083 Compare October 2, 2024 17:00
@9999years
Copy link
Collaborator Author

I am not sure this is the right direction of travel -- why are we propagating the design flaw of reusing the options parsers for the ./Setup interface for the cabal-install interface?

I can't comment on the design here, but all the other cabal-install commands use this pattern.

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

Successfully merging this pull request may close these issues.

3 participants