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 more documentation to declaring conditionals. #141

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

Conversation

deech
Copy link

@deech deech commented Jan 1, 2017

No description provided.

Unlike with `.cabal` files compound predicates need to be in parens, for example:

when:
- condition: (!os(darwin) && !os(windows))
Copy link
Owner

@sol sol Jan 8, 2017

Choose a reason for hiding this comment

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

I believe the issue here is not that you have a "compound predicate" here. It's merely the !, which in YAML must be quoted when at the beginning of a string, say:

condition: "!os(darwin) && !os(windows)"

/usr/lib/x64

If conditionals are declared under different `when` clauses only the last one
will make it to the `.cabal` file:
Copy link
Owner

@sol sol Jan 8, 2017

Choose a reason for hiding this comment

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

I think it may not necessarily be the last one. This is again resulting from how YAML works. This is an object (think of it as a hash map), duplicate keys are not allowed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right that this behaviour is on the YAML side of things (and is actually wrong according to the spec: http://yaml.org/spec/1.1/#id932806), but I find it very useful that it behaves as it does, and would actually propose to add tests for this behaviour and file bug reports to https://hackage.haskell.org/package/yaml if the behaviour ever changes. Here are two examples why:

Aliases with overriding:

executables:
    strava-gear-cmdline:
        &default_executable
        main: strava-gear-cmdline.hs
        dependencies: [strava-gear]
        ghc-options:
            - -threaded

    strava-gear-web:
        <<: *default_executable
        main: strava-gear-web.hs

Predefined aliases:

executables:
    &default_executable
    dependencies: [strava-gear]
    ghc-options:
        - -threaded

executables:
    strava-gear-cmdline:
        <<: *default_executable
        main: strava-gear-cmdline.hs

    strava-gear-web:
        <<: *default_executable
        main: strava-gear-web.hs

The second example is a massive hack, but I'm not aware of any other way to do this given that hpack insists on printing warnings for unused fields even if I prefix them with underscores.

Copy link
Owner

Choose a reason for hiding this comment

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

@liskin Hey, interesting! How about we don't emit warnings for unknown top-level field that starts with an underscore? That way you can use fields that start with underscores for building kind of a hpack library.

I would certainly accept a patch. So if you think thats the way to go, then feel free to open a PR!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you go: #146 :-)

@sol
Copy link
Owner

sol commented Jan 8, 2017

@deech First thing first, hanks for taking the time to open a PR.

As it currently stands, it's more like "this surprised me, and this is the workaround". But those things are not really surprising, they result from how YAML works.

I'm not suggesting we should not address this. But, can we address this in a way so that the user gets an understanding of the underlying mechanics (YAML, in this case)?

@deech
Copy link
Author

deech commented Jan 8, 2017

Hi,
Thanks for hpack! I loveit.

Having read your comments I agree that this is a YAML and not an hpack issue. I'm obviously not as familiar with YAML but as a Cabal user it did trip me up for a few hours. How do you feel about amending the PR to acknowledge that these "quirks" are consistent with underlying markup format but might be confusing to newcomers?

@sol
Copy link
Owner

sol commented Jan 12, 2017

@deech You are welcome! Please give it a try!


becomes

if os(darwin)
Copy link

Choose a reason for hiding this comment

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

Minor note generally, but cabal library converts Darwin to osx

@sol
Copy link
Owner

sol commented Sep 2, 2018

@deech hpack now warns on duplicate fields. So I think the main source of error has been removed! From what I understand, what is left is still that ! needs quoting. I'll cherry-pick the corresponding parts from you PR when I have time to do so.

Base automatically changed from master to main January 19, 2021 21:43
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.

3 participants