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

show-build-info (lib:Cabal part) #6108

Merged
merged 5 commits into from
Jun 25, 2019
Merged

Conversation

hvr
Copy link
Member

@hvr hvr commented Jun 23, 2019

This is the lib:Cabal part extracted from PR #5954 by @fendor and squashed into two commits.


Please include the following checklist in your PR:

  • 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.

Please also shortly describe how you tested your change. Bonus points for added tests!

@hvr hvr requested a review from 23Skidoo June 23, 2019 22:52
-- @
-- { "cabal-version": "1.23.0.0",
-- "compiler": {
-- "flavor": "GHC",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What would tea-drinking-dragon say seeing this one!

Copy link
Member

Choose a reason for hiding this comment

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

The lib:Cabal data type is called CompilerFlavor... in principle we can change the name for 3.0.

mkComponentInfo (name, clbi) = JsonObject
[ "type" .= JsonString compType
, "name" .= JsonString (prettyShow name)
, "unit-id" .= JsonString (prettyShow $ componentUnitId clbi)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not mentioned in the module header docs, but this is an important bit!

@@ -0,0 +1,46 @@
-- | Utility json lib for Cabal
-- TODO: Remove it again.
Copy link
Collaborator

Choose a reason for hiding this comment

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

please explain. Make an issue, link to it.

TODOs in the code are almost never fixed "immediately".

Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments.

c -> error $ "ShowBuildInfo.getCompilerArgs: Don't know how to get "++
"build arguments for compiler "++show c
where
-- This is absolutely awful
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a ticket about this as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wish there was.

]

mkCompilerInfo = JsonObject
[ "flavour" .= JsonString (prettyShow $ compilerFlavor $ compiler lbi)
Copy link
Member

Choose a reason for hiding this comment

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

Spelling here disagrees with module header docs.

@23Skidoo
Copy link
Member

OK, let's merge now, but please expand comments as requested in the review feedback.

bgamari and others added 5 commits June 25, 2019 03:25
This allows users to get a JSON representation of various information
about how Cabal would go about building a package. The output of this
command is intended for external tools and therefore the format should
remain stable.
This commit builds upon the work of cfraz89 and completes
the lib:Cabal part of the show-build-info feature.

Co-authored-by: Chris Fraser <[email protected]>
@23Skidoo 23Skidoo force-pushed the pr/show-build-info-libcabal branch from 8dc0c73 to ac1fc0f Compare June 25, 2019 02:32
@23Skidoo 23Skidoo merged commit 7a02cda into master Jun 25, 2019
@23Skidoo 23Skidoo deleted the pr/show-build-info-libcabal branch June 25, 2019 02:32
@23Skidoo
Copy link
Member

Merged, thanks!

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.

6 participants