rock-core / tools-roby

The roby plan manager
Other
3 stars 11 forks source link

normalize some calls that use keyword arguments to use the ** splat #174

Closed doudou closed 4 years ago

doudou commented 4 years ago

One important tidbit introduced in this PR are the sanitize_keywords_to_array and sanitize_keywords_to_hash helpers. They handle inconsistencies in Ruby 2.7 (compared to previous Rubies) for calls that mix strings and symbols in the last hash, such as

provides MODEL, "some" => "mapping", some: keywords
g-arjones commented 4 years ago

I'm still not sure about how I feel about these changes in Ruby 2.7. I think the devs may have messed things up :(

(Rubocop violations. Aren't you using vscode anymore?)

doudou commented 4 years ago

(Rubocop violations. Aren't you using vscode anymore?)

I am. Now, with the 1000s of errors I get from the C++ plugin, I end up missing some errors :(

doudou commented 4 years ago

Also, it's passing on my install. Not sure what's going on

doudou commented 4 years ago

I think the devs may have messed things up :(

So do I. For reference: https://bugs.ruby-lang.org/issues/16853, https://discuss.rubyonrails.org/t/new-2-7-3-0-keyword-argument-pain-point/74980/13?u=doudou42

g-arjones commented 4 years ago

https://bugs.ruby-lang.org/issues/16853

Interesting. Definitely messed up then (either in 2.0 or in 2.7)

doudou commented 4 years ago

Not sure what's going on

Now I do. rubocop 0.86 has a breaking change on the CyclomaticComplexity cop. I'm going to stick to 0.85.1, which passes.

g-arjones commented 4 years ago

Did you remove that pin from rock-package_set ?

doudou commented 4 years ago

Did you remove that pin from rock-package_set ?

No, but the checks here are from travis, which does not care about Rock.

We have to see how to handle rubocop versions in the rock package set. It's not the first time there are breaking changes - which, to be fair in this case, are not wrong - the methods it flags are rather complicated. Now, it's a bit stressful to get test break because rubocop stops passing suddenly.

g-arjones commented 4 years ago

No, but the checks here are from travis, which does not care about Rock.

Won't that make your internal CI fail?

g-arjones commented 4 years ago

Now, it's a bit stressful to get test break because rubocop stops passing suddenly.

Maybe we should have a separate build stage that's for linting

doudou commented 4 years ago

Maybe we should have a separate build stage that's for linting

Would be good, yes. It's quite a bit of work, so I would have preferred something less intrusive :P

Actually, could use GitHub actions for that... rubocop does not need to have anything installed. Same with yard.

g-arjones commented 4 years ago

Actually, could use GitHub actions for that

Why not just travis?

doudou commented 4 years ago

Why not just travis?

I want to try out Github actions :P

doudou commented 4 years ago

(and Github advertised a ready-to-use "magic linting" action recently)

g-arjones commented 4 years ago

(and Github advertised a ready-to-use "magic linting" action recently)

:+1: