testdouble / grunt-jasmine-bundle

A "spec" grunt task for Jasmine that includes a standard pack of helpers (jasmine-given, jasmine-stealth, jasmine-only). Uses minijasminenode.
6 stars 4 forks source link

merging minijasminenode options #5

Closed searls closed 10 years ago

searls commented 10 years ago

Hey @jasonkarns -- I swear this used to work but I'm also a crazy person. Either way I'd really like it to work. Dumping it here while I keep working on lineman:

I'd like the target-level options to extend the task-level options like so:

    spec:
      options:
        minijasminenode:
          showColors: true

      e2e:
        options:
          minijasminenode:
            onComplete: (runner, log) ->
              browser?.quit()

Such that what gets passed to minijasminenode as the config contains both the onComplete and the showColors. Right now it's just shallow-ly overwriting showColors in this case.

searls commented 10 years ago

After reading the code I suspect that lack of deep merge is a limit of the grunt this.options() API

jasonkarns commented 10 years ago

Yeah, grunt's options api is shallow merge only. I don't think it could have ever worked because prior to the change, any options set at the task level were never read at all.

I'm torn on this because doing a deep merge would be nicer from the user's perspective. However, it wouldn't be idiomatic grunt so it could take people by surprise. There's already a bit of discussion on this topic, though it's a bit old: https://github.com/gruntjs/grunt/issues/738

I think it's probably fine to break grunt 'convention' as long as it's clearly documented.

searls commented 10 years ago

Yeah I've read the grunt issues on this. The earlier ones were funny b/c they considered deep-merge of config actively harmful, which made me laugh since Lineman has basically been "let's write really smart deep merges" for about 450 commits

searls commented 10 years ago

Closing out of :meh: