Silence warnings for unknown top-level fields starting with an underscore by liskin · Pull Request #146 · sol/hpack

liskin · 2017-01-12T14:28:18Z

This lets users declare standalone YAML anchors and reuse them using YAML
aliases later:

_: &mydeps
 - base
 - containers

_my_library_of_hpack_stuff:
 - &mydeps [base, containers]
 - &myflags [-threaded, -Wall]

Also, this pull request adds a test that field overriding works as
expected and extends the README with a section on not repeating
yourself using these YAML tricks.

See also #144, which is a followup (logically, but not chronologically)
to this and enables putting those _libraries_of_hpack_stuff to separate
files and including them in multiple package.yamls across a larger
codebase.

sol · 2017-01-22T03:12:36Z

README.md

+with an underscore, so you can declare global aliases too:
+
+```yaml
+_: &exe-ghc-options


Let's use a variation of the common convention where the field name and anchor name are the same:

_exe-ghc-options: &exe-ghc-options - ...

My main motivation here is that if the user uses _ multiple times we rely on the non-standard libyaml behavior. This is why I prefer an example that avoids this problem by including the reference name in the field name.

sol · 2017-01-22T03:16:40Z

test/Hpack/ConfigSpec.hs

+        name: n1
+        name: n2
+        |]
+        (packageName >>> (`shouldBe` "n2"))


Now that we ignore warnings for fields that start with an underscore, is this still critical? I think it's better not to rely on that behavior. Please remove this test if you agree.

It is still critical because otherwise merge and override (d10a66d#diff-04c6e90faac2675aa89e2176d2eec7d8R194) wouldn't work.

My assumption was that this will always work and is not violating the spec. But then again, I'm not sure.

I'm not really sure how to interpret the spec regarding this, but I think that testing for the merge&override case specifically instead of this double definition would be a good idea anyway. :-)

Yes, I'm ok with that.

sol · 2017-01-22T03:30:38Z

src/Hpack/Config.hs

      fields = fieldNames (Proxy :: Proxy a)
+      ignoreUnderscored
+        | warnUnderscoredUnknownFields (Proxy :: Proxy a) = id
+        | otherwise = filter (not . isPrefixOf "_")


I like your code, it's quite elegant and extensible if we ever want to change the behavior in the future. Yet, being an advocate of TDD, I usually tend to go with the simplest solution that solves the problem and defer more clever solutions until we actually need them.

Further down we have a line

mkPackage dir (CaptureUnknownFields unknownFields globalOptions@Section{sectionData = PackageConfig{..}}) = do

Here you could make the changes with a two liner (using view patterns):

ignoreUnderscored = filter (not . isPrefixOf "_") mkPackage dir (CaptureUnknownFields (ignoreUnderscored -> unknownFields) globalOptions@Section{sectionData = PackageConfig{..}}) = do

Oh, I had almost the exact same code but then I realized we agreed on top-level unknown fields and rewrote it this way. :-(
But sure, I can simplify this.

I think the code I suggested will behave exactly the same way as your code (that is only ignore top-level fields that start with an underscore).

It won't, it will ignore this: https://github.com/sol/hpack/pull/146/files/d10a66db70aff2c225622fcdda929d15d92f7312#diff-af92d09f3a5eb3bf28df295b3a1fa13fR327 as well and I didn't consider that a top-level field. But I really don't have any strong feelings about this. If you think the simplification is worth it, let's do it that way. :-)

Ah, I see, it's because we treat unknown fields from top-level conditionals the same way as unknown top-level fields. In that case let's go with your solution.

Given that, there is only one more thing I would ask. Can we rename warnUnderscoredUnknownFields to ignoreUnderscoredUnknownFields and inverse the bools. That code is not really concerned with warnings, but rather with collecting unknown field names. Later we decide to turn them into warnings, but we could as well turn them into e.g. errors at some later point.

sol · 2017-01-22T03:32:05Z

test/Hpack/ConfigSpec.hs

        |]
        (`shouldBe` [
-          "Ignoring unknown field \"bar\" in package description"
+          "Ignoring unknown field \"_qux\" in package description"


If you go with the two-liner solution this test case becomes redundant.

As discussed we actually want to keep this test case.

…core This lets users declare standalone YAML anchors and reuse them using YAML aliases later: ```yaml _mydeps: &mydeps - base - containers ``` or ```yaml _my_library_of_hpack_stuff: - &mydeps [base, containers] - &myflags [-threaded, -Wall] ```

Before documenting how YAML can be abused to define common fields and reference them elsewhere, possibly overriding some, we should test that Data.Yaml actually behaves the way we need (that is, inversely to what YAML 1.1 spec says: http://yaml.org/spec/1.1/#mapping/syntax).

liskin · 2017-01-23T16:51:20Z

I pushed an updated version that hopefully addresses everything we agreed upon.

liskin mentioned this pull request Jan 12, 2017

Add more documentation to declaring conditionals. #141

Open

sol requested changes Jan 22, 2017

View reviewed changes

liskin added 3 commits January 23, 2017 17:47

Add a "Not repeating yourself" section to README

309d9f4

liskin force-pushed the ign-underscored-fields branch from d10a66d to 309d9f4 Compare January 23, 2017 16:49

sol approved these changes Jan 24, 2017

View reviewed changes

sol merged commit b106969 into sol:master Jan 24, 2017

liskin deleted the ign-underscored-fields branch January 24, 2017 14:55

Conversation

liskin commented Jan 12, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sol Jan 22, 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 23, 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!

liskin commented Jan 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sol Jan 22, 2017 •

edited

Loading

sol Jan 23, 2017 •

edited

Loading