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

Omit false conds #404

Merged
merged 2 commits into from
Nov 4, 2020
Merged

Omit false conds #404

merged 2 commits into from
Nov 4, 2020

Conversation

sol
Copy link
Owner

@sol sol commented Nov 4, 2020

No description provided.

@sol sol force-pushed the omit-false-conds branch 2 times, most recently from ce9ee11 to cb3c204 Compare November 4, 2020 13:33
@sol sol marked this pull request as ready for review November 4, 2020 13:34
@sol sol requested a review from tfausak November 4, 2020 13:34
@sol sol self-assigned this Nov 4, 2020
@@ -394,6 +412,8 @@ becomes
else
ghc-options: -O0

**Note:** Conditionals with `condition: false` are omitted from the generated
`.cabal` file.
Copy link
Owner Author

Choose a reason for hiding this comment

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

@tfausak do you see any potential issues with this behavior. One reason why I'm eager to do this is that it will make the fix for #400 less convoluted.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It also gives us cleaner .cabal files, which doesn't hurt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems safe to me!

Copy link
Collaborator

@tfausak tfausak 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!

Along the same lines, would it make sense to flatten conditionals where the value is always true?

README.md Outdated
@@ -35,6 +35,25 @@ at the Singapore Haskell meetup: http://typeful.net/talks/hpack

## Documentation

### Handling of `Paths_` modules

Cabal generates a `Paths_` module fore every package. By default Hpack adds
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Cabal generates a `Paths_` module fore every package. By default Hpack adds
Cabal generates a `Paths_` module for every package. By default Hpack adds

library:
when:
- condition: false
other-modules: Paths_name # substitute name with the package name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we include a note here about how Cabal generates these modules names? I think it replaces hyphens with underscores. I'm not sure if it does anything else.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's the only thing I'm aware of. Ideally, I would like to link to the Cabal docs on Path_, however, I couldn't find anything.

@phadej do you know if the Cabal docs cover Paths_ anywhere?

@@ -394,6 +412,8 @@ becomes
else
ghc-options: -O0

**Note:** Conditionals with `condition: false` are omitted from the generated
`.cabal` file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems safe to me!

String s -> return (Cond $ T.unpack s)
Bool True -> return (Cond "true")
Bool False -> return (Cond "false")
String c -> return (CondExpression $ T.unpack c)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this means someone could avoid this new filtering behavior by explicitly passing a string. Like this:

when:
  - condition: 'false'

That's probably fine, and may in fact be desired as a workaround.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, exactly, that's what I thought.

@sol
Copy link
Owner Author

sol commented Nov 4, 2020

would it make sense to flatten conditionals where the value is always true

Possibly, however, I'm not going to do this for now.

@sol
Copy link
Owner Author

sol commented Nov 4, 2020

@tfausak thanks for the review. I'm going to merge.

@sol sol force-pushed the omit-false-conds branch from cb3c204 to 6dfb49e Compare November 4, 2020 16:13
@sol sol merged commit 6dfb49e into master Nov 4, 2020
@sol sol deleted the omit-false-conds branch November 4, 2020 16:13
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.

2 participants