Replace '-' with '_' in method names #373

masayag · 2018-12-08T21:22:35Z

When names of methods aren't conventional, a minor tweak can allow using
the auto-generated methods.

For instance, the CNI defines entity named NetworkAttachmentDefinition.
Its resource is network-attachment-definition, and to access the
resource a call should be made for:

/apis/k8s.cni.cncf.io/v1/namespaces/default/network-attachment-definitions/ovs-foreman

However, with the current code, the generated methods for that resource
rely on the reported resource definition:

{
  "name"=>"network-attachment-definitions",
  "singularName"=>"network-attachment-definition",
  "namespaced"=>true,
  "kind"=>"NetworkAttachmentDefinition",
  "verbs"=>["delete", "deletecollection", "get", "list", "patch", "create", "update", "watch"],
  "shortNames"=>["net-attach-def"]
}

And by that, the stored information for the entity looks like:

 <OpenStruct entity_type="NetworkAttachmentDefinition",
             resource_name="network-attachment-definitions",
             method_names=["networkattachmentdefinition", "network-attachment-definitions"]>

This produces unsupported generated method names such as get_network-attachment-definitions
and watch_network-attachment-definitions which prevents client to access that resource.

The PR proposes to replace dashes with underscores to allow generation
of legal methods names, regardless of the weird naming.

cben · 2018-12-08T22:06:04Z

Ooh, interesting find!
Can you add a test case? And a note in CANGELOG.
Also note rubocop complaint (bundle exec rake to run locally).

In this case, it's maybe better to rely on singularName which is more obviously symmetric with name, rather than hoping underscores inferred from capitalization of kind matches the dashes in name?
Though singularName is optional, so we'd also have to fallback onto kind, so maybe simpler as you wrote...
OK either way.

masayag · 2018-12-08T22:43:18Z

@cben in terms of reducing any potential of regressions, I'd like to stay with my proposed PR, when the impacts are clear to me.

Fixed travis complains (use 'tr' instead of 'gsub' for performance).

added test for that special case

masayag · 2018-12-08T22:47:52Z

@cben when can we expect for a new release that will include this fix ?

cben · 2018-12-09T13:45:51Z

Hopefully tomorrow.

Link to the weird CRD: https://github.com/intel/multus-cni/blob/ca15454d5b76f42ad1c9173959a32ec452536d40/examples/crd.yml
(original here: https://github.com/K8sNetworkPlumbingWG/multi-net-spec/blob/master/%5Bv1%5D%20Kubernetes%20Network%20Custom%20Resource%20Definition%20De-facto%20Standard.md)

abonas · 2018-12-09T14:00:15Z

lib/kubeclient/common.rb

@@ -156,7 +156,7 @@ def self.parse_definition(kind, name)
          method_names = [singular_underscores, singular_underscores + plural_suffix]
        else
          # Something weird, can't infer underscores for plural so just give them up


is the comment still valid? it might be read as confusing along with code that actually adds underscores

Pls let me know if the updated comment makes more sense or requires any improvements / being removed entirely.

yes it does, thanks :)
I would say that "something weird" part is too generic and redundant, but it was here before, so I'm not insisting on taking it out.

abonas · 2018-12-10T09:00:54Z

@cben I'm leaving the merge to you, don't want to interfere with your release plans :)

cben · 2018-12-10T09:31:59Z

test/test_common.rb

@@ -74,4 +74,11 @@ def test_format_datetime_with_time
    formatted = client.send(:format_datetime, value)
    assert_equal(formatted, '2018-04-30T19:20:33.000000000+00:00')
  end
+
+  def test_parse_definition_with_dashes_in_resource_name
+    method_names = Kubeclient::ClientMixin.parse_definition('TestNameWithDashes',\


Let's use a singular kind here — 'TestNameWithDash'

@cben fixed.

cben · 2018-12-10T09:48:35Z

test/test_common.rb

+    method_names = Kubeclient::ClientMixin.parse_definition('TestNameWithDash',\
+                                                            'test-name-with-dashes').method_names
+    assert_equal(method_names[0], 'testnamewithdash')
+    assert_equal(method_names[1], 'test_name_with_dashes')


Wait, I missed this before, so we'll have get_testnamewithdash method for single entity but get_test_name_with_dashes for the collection?
I don't like this.

One direction is for this case to want consistent in get_test_name_with_dash, get_test_name_with_dashes.

one way we could obtain this is to use [singular_underscores, name.tr('-', '_')]. IFF kind's word boundaries match the plural dashes, a bit tricky to detect this...

another way is to use singularName! IFF it's specified, AND we're in "something weird" case so we give up relying on kind. [singular_name.tr('-', '_'), name.tr('-', '_')].

another direction is to drop the dashes, giving us get_testnamewithdash, get_testnamewithdashes.

The first direction is prettier — after all somebody went to non-standard trouble 😉 😇 to give us word boundaries in both singular and plural case — but I'm not sure how to detect this situation reliably.

PS. I think predictable method names are more important consideration than pretty. A dumber algorithm may be better if it's easier to explain?

@cben, have a look at the updated version and see if that's make sense to have.
Added few examples to test.
Please let me know if you'd like to include more cases or alter this PR.

cben · 2018-12-10T13:28:26Z

lib/kubeclient/common.rb

-          # Something weird, can't infer underscores for plural so just give them up
-          method_names = [singular_name, name]
+          stripped_name = name.tr('-', '').tr('_', '') # clear separators
+          if stripped_name.start_with?(singular_name)


almost good, but maybe better check singular_underscores.start_with?(name.tr('-', '_')).
currently you can add this test: (wrong plurals intentional, correct "y" -> "ies" plural is broken #366 (comment))

SameUptoWordboundary sameup-toword-boundarys same_upto_wordboundary sameup_toword_boundarys

where the resulting single vs plural boundaries differ! because stripped_name erased that info.

Did you mean the opposite ? Since singular_underscores is shorten than name and fails for all tests.

name.tr('-', '_').start_with?(singular_underscores)

However, with this change, your suggested test case
SameUptoWordboundary sameup-toword-boundarys same_upto_wordboundary sameup_toword_boundarys fails.

This test (and other) passes without changing the proposed code.

Sorry, yes, opposite.

The test "case" was not really a suggested case to add as-is, just example of stripped_name.start_with?(singular_name) behavior, which I think is not good:

get_same_upto_wordboundary get_sameup_toword_boundarys

And I think desired behavior here is the last fallback of dropping the dashes:

get_sameuptowordboundary get_sameuptowordboundarys

Updated the PR to produce the desired naming. Add test with proper expectation for it.

When names of methods aren't conventional, there will be an attempt to produce method names based on the following logic: 1. If the plural name without separators starts with lower-cased 'kind', use underscores as separators (assuming plural has separators) 2. In case of a mismatch, remove any separators and plural name will be the lower-case of it. For instance, the CNI defines entity named NetworkAttachmentDefinition. Its resource is 'network-attachment-definition', so to access the resource a call should be made for: /apis/k8s.cni.cncf.io/v1/namespaces/default/network-attachment-definitions However, with the current code, the generated methods for that resource rely on the reported resource definition: { "name"=>"network-attachment-definitions", "singularName"=>"network-attachment-definition", "namespaced"=>true, "kind"=>"NetworkAttachmentDefinition", "verbs"=>["delete", "deletecollection", "get", "list", "patch", "create", "update", "watch"], "shortNames"=>["net-attach-def"] } And by that, the stored information for the entity looks like: <OpenStruct entity_type="NetworkAttachmentDefinition", resource_name="network-attachment-definitions", method_names=["networkattachmentdefinition", "network-attachment-definitions"]> This produces unsupported generated method names such as 'get_network-attachment-definitions' and 'watch_network-attachment-definitions' which prevents client to access that resource. Using the PR, the generated method names will be: 'get_network_attachment_definition' and 'get_network_definitions'.

masayag · 2018-12-19T13:10:00Z

@cben should this be merged to master first or should I create a PR for 4.1 branch ?

masayag force-pushed the replace_dash_with_underscores branch from 93b8289 to 6b0ad50 Compare December 8, 2018 21:47

masayag force-pushed the replace_dash_with_underscores branch from 6b0ad50 to 50dfcc2 Compare December 8, 2018 22:43

abonas reviewed Dec 9, 2018

View reviewed changes

masayag force-pushed the replace_dash_with_underscores branch from 50dfcc2 to 6df71c5 Compare December 9, 2018 15:09

abonas approved these changes Dec 10, 2018

View reviewed changes

cben reviewed Dec 10, 2018

View reviewed changes

masayag force-pushed the replace_dash_with_underscores branch from 6df71c5 to a7a0393 Compare December 10, 2018 09:33

cben reviewed Dec 10, 2018

View reviewed changes

masayag force-pushed the replace_dash_with_underscores branch from a7a0393 to 4ea2dde Compare December 10, 2018 12:30

cben mentioned this pull request Dec 10, 2018

Simpler derivation of singular & plural method names #366

Merged

1 task

cben reviewed Dec 10, 2018

View reviewed changes

masayag force-pushed the replace_dash_with_underscores branch from 4ea2dde to 59e1d72 Compare December 11, 2018 05:51

masayag force-pushed the replace_dash_with_underscores branch from 59e1d72 to a43c9cd Compare December 11, 2018 06:01

masayag mentioned this pull request Dec 11, 2018

Volume name should respect restrictions fog/fog-kubevirt#47

Merged

cben mentioned this pull request Dec 12, 2018

Underscores disappeared for y -> ies plurals #376

Closed

masayag mentioned this pull request Dec 12, 2018

Support networks and interfaces fog/fog-kubevirt#50

Merged

cben mentioned this pull request Dec 18, 2018

Forward-port changes from 4.1.1 #380

Merged

masayag closed this Dec 26, 2018

masayag deleted the replace_dash_with_underscores branch December 26, 2018 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace '-' with '_' in method names #373

Replace '-' with '_' in method names #373

masayag commented Dec 8, 2018

cben commented Dec 8, 2018

masayag commented Dec 8, 2018

masayag commented Dec 8, 2018

cben commented Dec 9, 2018 •

edited

Loading

abonas Dec 9, 2018

masayag Dec 9, 2018

abonas Dec 10, 2018

abonas commented Dec 10, 2018

cben Dec 10, 2018

masayag Dec 10, 2018

cben Dec 10, 2018

masayag Dec 10, 2018

cben Dec 10, 2018

masayag Dec 10, 2018

cben Dec 10, 2018

masayag Dec 11, 2018

masayag commented Dec 19, 2018

Replace '-' with '_' in method names #373

Replace '-' with '_' in method names #373

Conversation

masayag commented Dec 8, 2018

cben commented Dec 8, 2018

masayag commented Dec 8, 2018

masayag commented Dec 8, 2018

cben commented Dec 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonas commented Dec 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayag commented Dec 19, 2018

cben commented Dec 9, 2018 •

edited

Loading