bump rubocop, fix offenses, use 2-space indent #223
Conversation
|
@cben can you review this? Thanks. |
| @@ -426,8 +422,8 @@ def load_entities | |||
| @entities = {} | |||
| fetch_entities['resources'].each do |resource| | |||
| next if resource['name'].include?('/') | |||
| resource['kind'] = Kubeclient::Common::MissingKindCompatibility | |||
| .resource_kind(resource['name']) if resource['kind'].nil? | |||
| resource['kind'] ||= | |||
There was a problem hiding this comment.
This will also use the falback mapping if it's false. Should never be a boolean, either a string or absent. And if it could be false, using mapping is better. => OK
| resource_version = | ||
| result.fetch('resourceVersion') do | ||
| result.fetch('metadata', {}).fetch('resourceVersion', nil) | ||
| end |
There was a problem hiding this comment.
@cben can you review the change above for correctness. Thanks.
There was a problem hiding this comment.
There is a mild difference:
given result == {'resourceVersion' => nil, 'metadata' => {'resourceVersion' => 10}},
old code would take the 10, while new code returns nil — fetch runs its block only when the key is entirely absent.
Don't know if this matters (can both resourceVersion and metadata.resource be present but one be nil?), and don't know which behavior is better.
Anyway I think this would match old logic exactly:
resource_version = result.fetch('resourceVersion', nil)
resource_version ||= result.fetch('metadata', {}).fetch('resourceVersion', nil)
[Well not exactly exactly :-) This still would still differ in handing of false, but it sounds safe to assume it's not a boolean.]
| status: 200) | ||
| .to_return( | ||
| body: open_test_file('core_api_resource_list.json'), status: 200 | ||
| ) |
There was a problem hiding this comment.
I must say I find the previous style in this particular case more readable.
2 lines became 3 but the actual arguments got crammed on one line.
IMHO the natural progression as lines get too long and require splitting is from:
func(arg1, arg2)
to
func(long_argument1,
long_argument2)
to
a_long.function_call(
long_argument1,
long_argument2
)
IIUC many of your changes are forced by Rubocop forbidding the 2nd option, so you compacted to 1st or expanded to 3rd, right?
I don't feel strongly about this (and I like most of your changes!) but I wonder if enforcing this is beneficial. Is there a Rubocop setting that would make it accept the 2nd form, leaving it to human discretion?
| end | ||
| assert_equal(expected_msg, exception.message) | ||
| end | ||
|
|
||
| def test_init_username_and_bearer_token | ||
| expected_msg = 'Invalid auth options: specify only one of username/password,' \ | ||
| ' bearer_token or bearer_token_file' | ||
| ' bearer_token or bearer_token_file' |
There was a problem hiding this comment.
consider dropping the start of the string to a new line (as you did elsewhere), to keep alignment.
|
LGTM. Thanks for the big cleanup! |
|
@cben can you please double-check that any extra change (other than indentation) is correct? (E.g. as you did for the one about If everything is alright then we'll merge. Thanks. |
|
Already did. I saw only 2 non-formatting changes, and I think they're both
OK.
(unless you think we might both resourceVersion and
metadata.resourceVersion simultaneously,
with one being nil?)
|
bump rubocop, fix offenses, use 2-space indent # Conflicts: # .rubocop.yml # lib/kubeclient/version.rb
Backporting ManageIQ#223 bumped to rubocop with different defaults. These rubocop errors are fixed on master by ManageIQ#253 and ManageIQ#269 but we didn't backport those.
Backporting ManageIQ#223 bumped to rubocop with different defaults. These rubocop errors are fixed on master by ManageIQ#253 and ManageIQ#269 but we didn't backport those.
@abonas
fixing weird indents with old rubocop version did not work ... so bumping ...
found a few new things (frozen constants / weird indents / always use raise)
to me these all look more readable now ... let me know which ones you don't like and I'll change them