boc-tothefuture / openhab-jruby

JRuby Libraries for Openhab
Eclipse Public License 2.0
6 stars 9 forks source link

feat(items): item builder DSL #648

Closed ccutrer closed 1 year ago

jimtng commented 1 year ago

Have you considered calling it create instead of build?

boc-tothefuture commented 1 year ago

I am having trouble following what this does - (there seems to be a couple changes in here and I am on mobile right now)

Is this an internal change only? Or are we adding to the DSL?

jimtng commented 1 year ago

@boc-tothefuture this PR adds to the DSL the ability for users to create and delete items from code. This should resolve #398

boc-tothefuture commented 1 year ago

Ok. I think anything that impacts the DSL should at least have a cucumber test. I know we moved some things to rspec that were internal testing but the actual DSL, imo, needs the rule facing tests.

Regardless of catching use facing regressions, I find it a lot harder to understand what we are adding to the actual language when it isn't in a cucumber test. I thought we discussed that in the PR or discussion around rspec, but I might be wrong.

WDYT?

ccutrer commented 1 year ago

Have you considered calling it create instead of build?

Nope, never crossed my mind. If the method were to only create a single item, it might make sense, but it's to enter the "builder DSL", a sort of sub-DSL to create multiple items.

Ok. I think anything that impacts the DSL should at least have a cucumber test. I know we moved some things to rspec that were internal testing but the actual DSL, imo, needs the rule facing tests.

Regardless of catching use facing regressions, I find it a lot harder to understand what we are adding to the actual language when it isn't in a cucumber test. I thought we discussed that in the PR or discussion around rspec, but I might be wrong.

WDYT?

I see zero value in writing tests for this in Cucumber. There are a very few things that aren't really testable in RSpec, but those are few and far between. As for what's in the DSL or not, that should be properly documented in the code (and thus the yardocs). You can see a preview of other tests I've converted to RSpec, and other new features I've written with only RSpec tests in my integration branch: https://github.com/ccutrer/openhab-jruby/commits/integration (warning: don't rely on any particular commit there - it's subject to rebasing and force pushes at any time).

jimtng commented 1 year ago

When I hear builder it makes me think of the builder design pattern which seems a little bit different here.

I see it more naturally with create, e.g.:

    items.create do
      switch_item 'MySwitch1', autoupdate: true
      switch_item 'MySwitch2', autoupdate: false
      switch_item 'MySwitch3'
    end

or even:

    items.create do
      group_item 'MyGroup' do
        self.name_base = 'Family'
        self.label_base = 'Family Room '

        switch_item 'Lights_Switch', 'Lights'
        switch_item 'Lamps_Switch', 'Lamps'
      end
    end

On another note, I noticed you support alexa and homekit, but not ga for google assistant. It could be added later, I suppose, and yes currently one can use the generic metadata for the time being.

ccutrer commented 1 year ago

Builder pattern is exactly the model and inspiration for the current name. Except your link is a very Java-ish style. https://github.com/jimweirich/builder is a prime example of a more Ruby-ish style. By taking a block and using instance eval it's far clearer how to build nested structures than using the chained method style.

ccutrer commented 1 year ago

https://nokogiri.org/rdoc/Nokogiri/XML/Builder.html is another example.

boc-tothefuture commented 1 year ago

I see zero value in writing tests for this in Cucumber. There are a very few things that aren't really testable in RSpec, but those are few and far between.

It is much less about what is or isn't testable. One value, for me, is that I can rapidly see the user experience of this feature. I find that the cucumber test much more clearly articulates the user experience.

There seems to be a difference of opinion here about our testing strategy. I recommend that you open a new issue with your proposed approach to testing (including principles of what framework is used where, how that impacts presently written tests, etc). We can discuss and reach a consensus/conclusion in that issue.

ccutrer commented 1 year ago

There seems to be a difference of opinion here about our testing strategy. I recommend that you open a new issue with your proposed approach to testing (including principles of what framework is used where, how that impacts presently written tests, etc). We can discuss and reach a consensus/conclusion in that issue.

https://github.com/boc-tothefuture/openhab-jruby/discussions/632