Allow to "!include" yaml files by liskin · Pull Request #144 · sol/hpack

liskin · 2017-01-09T22:52:47Z

There seems to be little reason not to allow this. Security can hardly be argued here as cabal custom setups and template haskell are far more powerful in this regard.

So anyway, here's an example of usage:

package.yaml:

# ...

tests:
    hlint: !include "../common/hlint.yaml"

hlint.yaml:

source-dirs: test
main: hlint.hs
ghc-options:
    -Wall
    -threaded
    -with-rtsopts=-N
dependencies:
    - base
    - hlint

Also, due to how Data.Yaml works, this can be abused to provide entire libraries of snippets:

package.yaml:

# if "xxx:" is used instead of a known field, a warning is emitted :-(
name: !include "../common/lib.yaml"

name: example1
version: '0.1.0.0'
synopsis: Example
<<: *legal

<<: *defaults

library:
    source-dirs: src

tests:
    hlint: *test_hlint

lib.yaml:

- &legal
    maintainer: Some One <someone@example.com>
    copyright: (c) 2017 Some One
    license: BSD3

- &defaults
    dependencies:
        - base
        - containers
    ghc-options:
        - -Wall
        - -Werror

- &test_hlint
    source-dirs: test
    main: hlint.hs
    dependencies: [hlint]

sol · 2017-01-12T01:39:12Z

In general, I'm open to this change. But I would like to get some more feedback from hpack users first, just to be sure that there are no concerns.

stack includes hpack @mgsloan @snoyberg

Also @soenkehahn any comments?

sol · 2017-01-12T01:44:47Z

@liskin Also, regarding the build failure, apparently the yaml library has different show instances depending on GHC version now? We might need CPP to fix this. Can you look into that too?

liskin · 2017-01-12T11:40:30Z

The build failure seems to be this: haskell/directory#44 (comment)
I have no idea which version of directory tinc brings in scope but it's likely the buggy one. Shall I go the #if MIN_VERSION_directory(1, 2, 3) route or can we just upgrade directory in Travis builds?

sol · 2017-01-24T00:57:15Z

@liskin tinc tries to use the directory version that comes with GHC if possible.

sol · 2017-01-24T01:00:19Z

@liskin I'm happy to merge this if you can address the following points:

dig into the build failure
add documentation
as per Keep a changelog #149 add a changelog entry

liskin · 2017-01-24T15:49:10Z

I pushed a workaround for the failing tests, but it's (unsuprisingly) a bit ugly. Maybe I should submit it to the yaml package instead. @snoyberg what do you think?

snoyberg · 2017-01-24T15:51:09Z

I think it makes sense for yaml.

liskin · 2017-01-24T16:25:33Z

Okay, I'll prepare a pull req there later this evening. :-)

liskin · 2017-01-24T23:01:11Z

So, here you go: snoyberg/yaml#104

@sol But I guess we still want the workaround here as well, right? Anyway, I'll write the docs and changelog tomorrow.

sol · 2017-01-25T03:12:00Z

@liskin If we can fix this in yaml we don't need a workaround here anymore. I'm fine if the tests pass with the latest version of yaml

I'm willing to accept that the exception behavior will be different with older versions of yaml + directory for the sake of code simplicity.

liskin · 2017-01-25T12:30:50Z

Okay, yaml 0.8.21.2 is on hackage so I dropped the ugly commit, added documentation, enhanced the changelog and added an entry for this.

liskin · 2017-01-26T15:50:03Z

Finally, green build checks! :-)

sol · 2017-01-27T12:23:46Z

test/Hpack/ConfigSpec.hs

+          - A2
+        |]
+        )
+        (packageAuthor >>> (`shouldBe` ["A1", "A2"]))


This two things are united tested in the yaml library. I vote for removing them here.

Oh, I considered it part of "hpack specification" and naturally wrote a hspec test for that. :-)

I don't fully agree with @jbrains, but for completeness: http://blog.thecodewhisperer.com/permalink/integrated-tests-are-a-scam

"Interesting" article. Too bad it offers no alternatives. Not having integration tests at all and just having it blow up in production is, in my humble opinion, not a better alternative. :-)

But I can agree that slow tests are really bad, but I think that's a technical problem, not a fundamental one. For a long time I wanted qemu to support superfast copy on write snapshots via forking (with readonly file/disk-backed storage, obviously) which I think could help a lot, but never had enough time/budget to implement that.

Anyway, I dropped the two tests.

sol · 2017-01-27T12:24:05Z

src/Hpack/Yaml.hs


-import           Data.Yaml
+import           Data.Yaml hiding (decodeFile, decodeFileEither)
+import           Data.Yaml.Include


sol · 2017-01-27T12:24:24Z

README.md

+  source-dirs: test
+  main: hlint.hs
+  dependencies: [hlint]
+```


sol · 2017-01-27T12:30:53Z

CHANGELOG.md

+All notable changes to this project will be documented in this file.
+
+The format is based on [Keep a Changelog](http://keepachangelog.com/)
+and this project adheres to [Haskell Package Versioning Policy](https://pvp.haskell.org/).


I agree with the PVP for my code. But the current version of the PVP also contains this wording:

Note that modifying imports or depending on a newer version of another package may cause extra orphan instances to be exported and thus force a major version change.

Some people interpret this as: In theory one of your dependencies (e.g. directory) could add an orphan instance at some point in the future, for that reason following the PVP means you have to specify upper bounds for all your dependencies.

Even though this reasoning is sound in theory, I'm still not going to do that. I simply don't have the bandwidth to deal with version bumps of all the dependencies of all my projects all the time. My experience is that upper bounds cause more pain than they help anything. In the rare case that things actually break, I rather invest my time to fix things.

Let's just remove this paragraph.

sol · 2017-01-27T12:47:45Z

CHANGELOG.md

+  [#67](https://github.com/sol/hpack/issues/67)
+
+[Unreleased]: https://github.com/sol/hpack/compare/0.16.0...HEAD
+[0.16.0]: https://github.com/sol/hpack/compare/0.15.0...0.16.0


I'm on the fence with the hyperlinks in the change log. I agree that it looks nice, but I may not have the bandwidth to keep up with doing it like this in the future. Let's keep it that way for now and see.

sol · 2017-01-27T12:48:21Z

CHANGELOG.md

+## [0.16.0] - 2017-01-11
+### Added
+- Warn when `name` is missing [#109](https://github.com/sol/hpack/issues/109)
+- Support globs in `c-sources`


I think this change never made it into the README. Extra points if you can fix that ;)

Basically we want this "Accepts glob patterns" comments, same to what we have for other fields

sol · 2017-01-27T12:51:39Z

@liskin Hey, thanks for taking care of this and all the other issues that spawned from it. I think this is a pretty awesome change.

If you can address my bikeshedding, it's good for merge.

There seems to be little reason not to allow this. Security can hardly be argued here as cabal custom setups and template haskell are far more powerful in this regard. See the changes in `README.md` for examples of usage.

liskin · 2017-01-28T10:20:51Z

And now the Windows build blows up when building happy. That can't be my fault, can it?

This should fix the remaining CI failures.

liskin · 2017-01-28T13:30:37Z

Pushed again, everything's green again.

sol · 2017-01-29T02:48:23Z

@liskin Thanks a lot!

In general, my bandwidth is limited for medical reasons. I try to keep up with important stuff, but I'm grateful for any help. If you want to further help with things, then go ahead and bump the version. Also what is your Hackage account name?

liskin · 2017-01-30T00:07:22Z

#154

I'm TomasJanousek on Hackage.

liskin mentioned this pull request Jan 12, 2017

Silence warnings for unknown top-level fields starting with an underscore #146

Merged

liskin force-pushed the yaml-include branch 2 times, most recently from 1a29e70 to f8dccaa Compare January 24, 2017 15:47

liskin force-pushed the yaml-include branch from f8dccaa to b18a97b Compare January 25, 2017 12:28

sol reviewed Jan 27, 2017

View reviewed changes

src/Hpack/Yaml.hs

import Data.Yaml

import Data.Yaml hiding (decodeFile, decodeFileEither)

import Data.Yaml.Include

Copy link
Copy Markdown

Owner

sol Jan 27, 2017

Choose a reason for hiding this comment

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

👍

sol reviewed Jan 27, 2017

View reviewed changes

README.md

source-dirs: test

main: hlint.hs

dependencies: [hlint]

```

Copy link
Copy Markdown

Owner

sol Jan 27, 2017

Choose a reason for hiding this comment

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

👍

sol reviewed Jan 27, 2017

View reviewed changes

liskin added 3 commits January 28, 2017 10:57

Prettify changelog

43c4cd4

Mention c-sources globbing (sol#123) in README.md

13a6d17

Allow to "!include" yaml files

d99bf2f

There seems to be little reason not to allow this. Security can hardly be argued here as cabal custom setups and template haskell are far more powerful in this regard. See the changes in `README.md` for examples of usage.

liskin force-pushed the yaml-include branch from 0aebd27 to 4e2151b Compare January 28, 2017 10:12

Use yaml-0.8.21.2 for stack builds

9e981a5

This should fix the remaining CI failures.

liskin force-pushed the yaml-include branch from 4e2151b to 9e981a5 Compare January 28, 2017 12:04

sol merged commit 8524570 into sol:master Jan 29, 2017

liskin deleted the yaml-include branch January 29, 2017 23:09

Conversation

liskin commented Jan 9, 2017

Uh oh!

sol commented Jan 12, 2017

Uh oh!

sol commented Jan 12, 2017

Uh oh!

liskin commented Jan 12, 2017

Uh oh!

sol commented Jan 24, 2017

Uh oh!

sol commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liskin commented Jan 24, 2017

Uh oh!

snoyberg commented Jan 24, 2017

Uh oh!

liskin commented Jan 24, 2017

Uh oh!

liskin commented Jan 24, 2017

Uh oh!

sol commented Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liskin commented Jan 25, 2017

Uh oh!

liskin commented Jan 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sol Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sol Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sol commented Jan 27, 2017

Uh oh!

liskin commented Jan 28, 2017

Uh oh!

liskin commented Jan 28, 2017

Uh oh!

sol commented Jan 29, 2017

Uh oh!

liskin commented Jan 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sol commented Jan 24, 2017 •

edited

Loading

sol commented Jan 25, 2017 •

edited

Loading

sol Jan 27, 2017 •

edited

Loading

sol Jan 27, 2017 •

edited

Loading