Add better error messages for yaml selectors

[gshank]

resolves #2700

Description

Changed hologram to provide more information in the exceptions, and modified selector error messages to include yaml output and more information.

Checklist

• I have signed the CLA
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

[jtcohen6]

Nice work on these exceptions! I noted one more case that I think we should try to catch + raise.

I think I understand the connection to dbt-labs/hologram#35, insofar as when I took this PR for a spin and defined bad selectors I was more likely to get an eager, generic error rather than a more precise, descriptive one.

[jtcohen6]

Could you also add an exception + test for quite similar case, which I've seen a few folks run into:

selectors:
  - name: mixed_syntaxes
    definition:
      key: value
      method: tag
      value: foo
      union:
        - method: tag
          value: nightly
        - exclude:
          - method: tag
            value: hourly

The issue here is that contradictory dict arguments are being passed to definition, and it's not at all clear which ones it's using.

[gshank]

Are we talking about the "key: value" piece? or having a union in a dictionary without a 'union' above it? I was under the impression that the above should be:

selectors:
    - name: mixed_syntaxes
      definition:
         union:
           - method: tag
              value: foo
            - method: tag
               value: nightly
            - exclude:
                 method: tag
                 value: hourly

So having a 'union' in a single dictionary should be an error, and having an invalid keyword 'key' should be an error?

[jtcohen6]

Right on—the way you've defined it is how it should be. I've seen users write the version I put above, which neither throws an error nor works the way they'd expect. I think we should check to see that definition is one of:

• string
• dict w/ 1 argument (union, intersection, or arbitrary key: value)
• dict w/ 2 arguments (must be method + value)

and throw an exception if it doesn't meet one of those three criteria. Does that sound reasonable?

[gshank]

I think there are quite a few other arguments that you can put in a dict, such as 'children', 'parents', etc. What I did and pushed was to check that if there is a union or intersection operator in the root level, there's not also a 'method' key. The way the code worked was that it would just. ignore additional dict keys if there was a union or intersection, so that takes the "way it works" and creates an error.

Let me know if you see any other situations that are problematic.

[jtcohen6]

Ah, that's a really good point about children, parents, child_depth, etc. Silly of me to forget, sorry.

That sounds about right to me. Would it be possible to generalize this:

check that if there is a union or intersection operator in the root level, there's not also a 'method' key.

To be: "if there is a union or intersection operator in the root level, there should be no other dict arguments."

OR: "There should be only one dict argument, unless method is one of the arguments."

I think this would also avoid the weirdness of something like:

selectors:
    - name: multiple_sets
      definition:
         intersection:
           - method: tag
             value: foo
           - method: tag
             value: bar
         union:
           - method: tag
             value: nightly
           - exclude:
              - method: tag
                value: hourly

If I understand correctly, the current behavior would be to take the first set and ignore the second set entirely. Really we should throw an error.

[gshank]

Yes, that would cover more situations. Will change.

[jtcohen6]

It looks like this now raises a much better error message:

selectors:
    - name: intersection_and_method
      definition:
         intersection:
           - method: tag
             value: foo
           - method: tag
             value: bar
         method: tag
         value: bar

 You cannot have both method and intersection keys in a root level selector definition:

Nice!

The multiple_sets definition above, however, still doesn't raise an error.

[gshank]

Strange. For the multiple_set example I get: Only a single 'union' or 'intersection' key is allowed in a root level selector definition; found intersection,union.

[jtcohen6]

This now works wonderfully!

Could not read selector file data: Runtime Error
    Only a single 'union' or 'intersection' key is allowed in a root level selector definition; found intersection,method,value.

I'm sure this was on me for not pulling the latest remote changes.

[jtcohen6]

Thanks for this! I know that we'll continue to make iterative improvements on YAML selectors. I think this is a big step toward avoiding unnecessary surprises.

[jtcohen6]

This now works wonderfully!

Could not read selector file data: Runtime Error
    Only a single 'union' or 'intersection' key is allowed in a root level selector definition; found intersection,method,value.

I'm sure this was on me for not pulling the latest remote changes.

[jtcohen6]

Incredibly nit-picky and totally up to you:

Suggested change

	keys = ",".join(definition.keys())
	keys = ", ".join(definition.keys())