Positional and params support

[tsontario]

Goals

Aims to close #312 (Harmonize positional vs keyword namespace args).

For now, I'm looking for validation of the approach or whether or not we want to try solving this a different way.

Proposed Solution

This solution leverages (abuses?) the fact that ruby captures all named parameters in **kwargs if all other parameters are provided default values. By doing this, we add the ability to supply named parameters to get, delete, patch, etc. while still supporting the previous unnamed parameter implementation. This should provide the means to gracefully deprecate unnamed parameters for these methods moving forward. Once that's done, we can explicitly put the named parameters in the method signatures and clean up the logic for dealing with both cases.

[cben]

Thanks!
Nice, I like the direction!
👍 to allowing keyword name: as well, reduces cognitive load.

• What if one doesn't pass name at all?
• What if one passes unknown keyword args?
• What if one passes both positional and keyword name or namespace?

I'll look later at existing tests, I might want to add some non-VCR coverage, esp. without namespace (test_node.rb, test_namespace.rb), esp. combined with options such as as:.

[cben]

I'd like unrecognized options to still raise an error.
Perhaps it's possible to have Ruby do it for us with

def delete_entity(resource_name, positional_name = nil, positional_namespace = nil, 
                  name: nil, namespace: nil, delete_options: {})
  ... positional_namespace || namespace ...
  ... positional_name || name ...

Otherwise, manually pick out known stuff from **opts and assert nothing remains:

def delete_entity(resource_name, name = nil, namespace = nil, delete_options: {}, **opts)
  # bonus: this errors if both positional and keyword given
  name = name || opts.delete(:name)
  namespace = namespace || opts.delete(:namespace)
  # omitted `.to_h` don't see why its needed
  raise ArgumentError, "unexpected keywords: #{opts.keys}" unless opts.empty?

[cben]

I suspect we can make a helper to deal with name & namespace, and keep others (as:, delete_options:) in regular named params.

Perhaps it can subsume build_namespace_prefix, computing all of ns_prefix + resource_name + "/#{opts[:name] || name}".

[cben]

Could this and existing test_guestbook_go.rb share the same cassette?
(either old or your new recording, but the whole point both calling styles should be equivalent)

[timothysmith0609]

I'd like unrecognized options to still raise an error.

Definitely agree with you there and it shouldn't be difficult to enforce this (via your suggestions or some similar mechanism). I think we can probably do away with **args and just default name:, namespace:, etc to nil. This will increase the complexity of the methods in the short term (via duplication of name/namespace in both positional and keyword sections) but provides a convenient deprecation path without excessive code additions.

Do you have an idea of what the deprecation timeline will look like and is there an established manner in communicating these changes? Do we just use the CHANGELOG or actually print warnings to users?

Regarding the tests, I wouldn't pay too much attention to them now as they're just there to sanity-check my solution. I'm conflicted about using the same cassette for both approaches since I prefer to fully encapsulate my tests, but I could be convinced that this case would benefit from your idea.

[cben]

So far we haven't bothered with active warnings, simply CHANGELOG + breaking change in major release. Better to add new way & document upcoming change ahead of that when possible, of course.
We could do warnings if desired.

Do you mean deprecating positional name, namespace?
Why?
I'm not sure there is payoff to justify the incompatibility. For observable future I'd keep the exisiting inconsistent support for positional.
We can encourage consistent keyword style in docs.

[tsontario]

Do you mean deprecating positional name, namespace?
Why?

I think I need some clarification here. My goal with this PR is to support both positional and keyword parameters for name, namespace in the hope that, at some later date, we will fully discontinue positional parameters. What I don't want is to support both positional/keywords in perpetuity. The change to keywords is clearly a breaking change and would be a feature of a major release.

To reiterate, I'm ok with supporting the inconsistent as-is for now, but at some point we should enforce keywords-only everywhere.

[cben]

Understood. I don't yet see a significant price to supporting both in perpetuity, but it's quite possible technical debt will pile up with time... We don't need to agree on that now as this is good direction for the code anyway 👍 😀

[tsontario]

I don't yet see a significant price to supporting both in perpetuity

For me, this PR is about finding a canonical way to pass the parameters to the client functions (whether that ends up being positional or params is not really important at this point). I'm uncomfortable providing multiple means of doing the same thing for too long, and want to do it here only as a means to provide a graceful deprecation path.

In particular, the means in which I want to go about this is to provide an around alias on top of the current get/watch/etc. operations that accepts both inputs and, once deprecation is complete, we can just eliminate those around aliases and the underlying methods will have the proper contract. Keeping that around for a long time is dangerous as there's a lot of indirection for very little gain (e.g. it's hard to see that we're in fact aliasing the methods in the first place)

[cben]

Can you resolve conflicts, and check rubocop?
(feel free to increase, or locally disable, limits like PerceivedComplexity. They're a useful reminder to humans but unsuitable as hard CI-enforced rules.)

This is good stuff and I'd like to land it; we can always improve later.

[timothysmith0609]

Apologies, I've been quite busy lately so this fell off my radar. I'll try to take a look at this in the next 2 weeks and iron everything out to get it in shape for release.

[tsontario]

Closing this as I don't foresee myself completing this work.