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

Add support for cabal.project fields to scripts #7997

Merged
merged 5 commits into from
Mar 2, 2022

Conversation

bacchanalia
Copy link
Collaborator

@bacchanalia bacchanalia commented Feb 19, 2022

Closes #5698

Currently, as with the executable block, this just reads it and does not attempt to validate that the fields set are relevant to scripts. I think this is probably fine, but I'm open to discussing it.

@bacchanalia bacchanalia marked this pull request as draft February 19, 2022 22:57
@bacchanalia bacchanalia marked this pull request as ready for review February 27, 2022 16:19
@@ -15,5 +15,7 @@ description: {
- The name of the generated script executable has been changed from "script" to
"cabal-script-<your-sanitized-script-name>" for easier process management.
- Reduce the default verbosity of scripts, so that the build output doesn't interfere with the script output.
- Scripts now support a project metadata block that allows them to use options
that would normally be set in a cabal.project file.
Copy link
Member

Choose a reason for hiding this comment

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

this just reads it and does not attempt to validate that the fields set are relevant to scripts. I think this is probably fine, but I'm open to discussing it.

a note in docs about that would be enough imo

Copy link
Member

Choose a reason for hiding this comment

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

And an issue about adding such validation would be great too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added an issue: #8024

Copy link
Member

Choose a reason for hiding this comment

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

Many thanks, a really good recap, maybe it worths to link the issue in the doc note (as list all the problematic fields would be too verbose)

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

great work as usual, many thanks
eager to see the amazing new script support released

-}

main :: IO ()
main = do
...

Where there cabal metadata block is mandatory and contains fields from a
Copy link
Member

Choose a reason for hiding this comment

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

This interjects the sentences "With a script that looks like: ... It can either be executed like any other script..." and the context is lost before we get to "It can either". Perhaps change the initial sentence to "A script may looks like this:", then your interjection, then "The script can either be executed like any other script...".

@bacchanalia bacchanalia requested a review from Mikolaj March 2, 2022 19:19
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

👌

@bacchanalia bacchanalia added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Mar 2, 2022
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.

Add support for (some) cabal.project-fields to cabal script-metadata
3 participants