Enabling rubo configs/12 20 23 #241

calderete · 2023-12-20T19:56:14Z

🤔 What's changed?

Regenerated rubcop todo and fixed warnings.
Added to rubocop config:

Metrics/AbcSize:
Max: 30

Metrics/MethodLength:
Max: 23

RSpec/ExampleLength:
Max: 6

⚡️ What's your motivation?

Helping out

🏷️ What kind of change is this?

🏦 Refactoring/debt/DX (improvement to code design, tooling, documentation etc. without changing behaviour)

♻️ Anything particular you want feedback on?

I added some configs to rubocop.yml for code length. Should those be refactored to fit those cops instead?

📋 Checklist:

I agree to respect and uphold the Cucumber Community Code of Conduct

This text was originally generated from a template, then edited by hand. You can modify the template here.

mpkorstanje · 2023-12-20T20:28:33Z

Hey! Thanks for wanting to help out!

I'll have to let Luke do the actual review. But for your pull request title and description please follow the guidance from https://cbea.ms/git-commit

It helps everyone to quickly see what your pull request is about.

ruby/lib/cucumber/ci_environment.rb

ruby/spec/cucumber/ci_environment/remove_user_info_from_url_spec.rb

olleolleolle · 2023-12-20T21:25:01Z

(And yeah, awesome that you made a PR!)

luke-hill

Few things to unpick or tweak if you can. Will chat on slack too.

ruby/.rubocop.yml

ruby/Gemfile

ruby/lib/cucumber/ci_environment.rb

ruby/lib/cucumber/ci_environment/variable_expression.rb

ruby/spec/cucumber/ci_environment/detect_ci_environment_spec.rb

luke-hill · 2023-12-21T09:55:47Z

ruby/spec/cucumber/ci_environment/variable_expression_evaluate_spec.rb

 result = evaluate(expression, { 'GO_SCM_MY_MATERIAL_PR_BRANCH' => 'ashwankthkumar:feature-1' })
 expect(result).to eq('feature-1')
 end

 it 'evaluates a complex expression' do
- expression = "hello-${VAR1}-${VAR2/(.*) (.*)/\\2-\\1}-world"
+ expression = 'hello-${VAR1}-${VAR2/(.*) (.*)/\\2-\\1}-world'
 result = evaluate(expression, {


If you're comfortable using multi-line here this can be fixed. Or I can tackle it at a later date

Sorry, I'm not sure what is meant by "multi-line" here

This stylistically is quite wild now, so ideally it should be on multiple lines. i.e.

( { } )

If you're not comfortable tidying it up, don't feel the need to fix it all. We can revisit these at a later date.

I guess I'm not understanding how the highlighted line could be refactored into multi line. The rest of that block is multi line. Do you mean the other blocks in this file?

As written above, the rubocop parser is detecting you want to have this right-aligned, which shifts all the code along. If you make it left aligned in the braces then it fixes this issue

luke-hill

Popped an update on to help you along if you have time.

ruby/Rakefile

luke-hill · 2024-01-05T12:09:44Z

ruby/lib/cucumber/ci_environment/variable_expression.rb

- variable = $1
- pattern = $2
- replacement = $3
+ variable = ::Regexp.last_match(1)


Not sure these need to be top-level namespaced

Tests pass when refactoring ::Regexp to just Regexp do you want me to change these to Regexp?

luke-hill · 2024-01-05T12:10:53Z

ruby/spec/cucumber/ci_environment/detect_ci_environment_spec.rb

@@ -3,15 +3,15 @@
 require 'cucumber/ci_environment'
 require 'json'

-describe 'detect_ci_environment' do
+describe Cucumber::CiEnvironment, '.detect_ci_environment' do


this should be in 2 blocks. So the top level should say

describe Cucumber::CiEnvironment

and then underneath it it should have the 2nd block

Do you mean refactor it to be ?

describe Cucumber::CiEnvironment do it 'detects CI environment' do <test code> end end

2nd line should be describe '.detect_ci_environment'

ruby/spec/cucumber/ci_environment/detect_ci_environment_spec.rb

luke-hill · 2024-01-05T12:12:31Z

ruby/spec/cucumber/ci_environment/remove_userinfo_from_url_spec.rb

 require 'cucumber/ci_environment'

-describe 'remove_user_info_from_url' do


Same as the above one.

luke-hill · 2024-01-05T12:13:02Z

ruby/spec/cucumber/ci_environment/variable_expression_evaluate_spec.rb

 require 'cucumber/ci_environment'

-describe 'Cucumber::CiEnvironment::VariableExpression.evaluate' do


This also needs to be in double blocks

luke-hill · 2024-01-05T12:13:44Z

ruby/spec/cucumber/ci_environment/variable_expression_evaluate_spec.rb

 require 'cucumber/ci_environment'

-describe 'Cucumber::CiEnvironment::VariableExpression.evaluate' do
- include Cucumber::CiEnvironment::VariableExpression


No need to amend this as this isn't the way to write these kinds of specs, we should be using isolated lets. But given this is already here we can ignore this and fix it later.

I am curious as to how the let refactor would work and what is the difference? From looking at this spec file I can see what it is testing, the inputs and expected outputs are readily seen. What is bad about this rspec file?

When you include something inside a describe block it pollutes the whole namespace. Dependent on how you ran / checked rubocop it will flag this to you. A better way it to use an isolated anonymous class inside a let so something like

let(:foo) do Class.new(inheritance) do include RequiredModule1 include RequiredModule2 end end

Now the inclusion is completely isolated inside the memoized let.

luke-hill · 2024-01-05T12:14:16Z

ruby/.rubocop_todo.yml

 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.

-# TODO: Oct '23 -> 10 files inspected, 63 offenses detected, 53 offenses autocorrectable


if you leave these stylistic markers in it'll show we're slowly getting more conformant. It'll also warn us if we're getting less conformant

luke-hill · 2024-01-05T12:16:17Z

ruby/spec/cucumber/ci_environment/variable_expression_evaluate_spec.rb

 result = evaluate(expression, { 'GO_SCM_MY_MATERIAL_PR_BRANCH' => 'ashwankthkumar:feature-1' })
 expect(result).to eq('feature-1')
 end

 it 'evaluates a complex expression' do
- expression = "hello-${VAR1}-${VAR2/(.*) (.*)/\\2-\\1}-world"
+ expression = 'hello-${VAR1}-${VAR2/(.*) (.*)/\\2-\\1}-world'
 result = evaluate(expression, {


This stylistically is quite wild now, so ideally it should be on multiple lines. i.e.

( { } )

If you're not comfortable tidying it up, don't feel the need to fix it all. We can revisit these at a later date.

Charles Alderete added 30 commits December 20, 2023 12:15

regenerated rubocop todo

d274f0d

hash indent and align offenses auto corrected

6168d60

empty line after guard clause auto corrected

a05c8c4

deprecated attribute: test files removed

88d7b48

space after comma auto corrected

ab37bb1

space around operators autocorrected

653b0aa

space before block braces auto corrected

05a5e2f

parenthese as group exp autocorrected

02d32a6

added metrics abc size and method length to rubo config

ff36118

redundant split regex auto corrected

7b24ea5

performanc regex match auto corrected

ae55f33

rspec be over eq autocorrected

55c8821

empty line after subject autocorrected

d7ff34e

rspec example length config added to rubocop config

6f8f99d

security io methods auto corrected

404724a

frozed string literal autocorrected

2e85680

style hash conversion auto corrected

a7abc0e

parentheses around condition auto corrected

e54bc37

style perl backrefs autocorrected

0226c0e

syle redundant return auto corrected

d26baf0

style redundant string escape auto corrected

513f516

style rescue modifier auto corrected

45c39f5

style rescue standard error auto corrected

1ee0d7f

style semicolon auto corrected

d02cab9

style slicing with range auto corrected

2639f8a

style string literals auto corrected

3ab0185

regenerated rubocop todo

ca9e483

style trailing comma in hash literal auto corrected

67a46f8

refactored spec file path

d0f566f

refactored rspec tests to pass rubocop

41597c3

Charles Alderete added 2 commits December 20, 2023 14:30

added desc to spec task

488a942

gemspec development dependencies fixed for rubocop

309b7a6

mpkorstanje requested a review from luke-hill December 20, 2023 20:28

olleolleolle reviewed Dec 20, 2023

View reviewed changes

ruby/lib/cucumber/ci_environment.rb Outdated Show resolved Hide resolved

olleolleolle reviewed Dec 20, 2023

View reviewed changes

ruby/spec/cucumber/ci_environment/remove_user_info_from_url_spec.rb Outdated Show resolved Hide resolved

Charles Alderete added 2 commits December 20, 2023 17:14

revert filename and method in rspec test 'remove_userinfo_from_url'

9f47e3e

replace hash parsing with dig()

916c89f

luke-hill reviewed Dec 21, 2023

View reviewed changes

Charles Alderete added 9 commits December 22, 2023 13:03

move metrics warnings back into rubocop todo

f30211b

move gem dev dependicies back into gemspec

cbb7db6

refactored detect revision method to use dig()

f98e674

silence gemspec dev dependency cop

59ad17e

revert endless range

67e0455

remove error handling from rescue block, regenerate rubocop todo

2a310bc

remove nil saftey

6a9562d

spec describe arg passes class method

8294604

spec describe arg passes class method

ac41931

luke-hill reviewed Jan 5, 2024

View reviewed changes

Charles Alderete added 2 commits January 11, 2024 10:53

use empty hash inline insstead of defining {} through let

6287011

update rake desc

003ab0f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling rubo configs/12 20 23 #241

Enabling rubo configs/12 20 23 #241

calderete commented Dec 20, 2023

mpkorstanje commented Dec 20, 2023

olleolleolle commented Dec 20, 2023

luke-hill left a comment

luke-hill Dec 21, 2023

calderete Dec 22, 2023

luke-hill Jan 5, 2024

calderete Jan 11, 2024

luke-hill Jan 11, 2024

luke-hill left a comment

luke-hill Jan 5, 2024

calderete Jan 11, 2024

luke-hill Jan 5, 2024

calderete Jan 11, 2024 •

edited

luke-hill Jan 11, 2024

luke-hill Jan 5, 2024

luke-hill Jan 5, 2024

luke-hill Jan 5, 2024

calderete Jan 11, 2024

luke-hill Jan 11, 2024

luke-hill Jan 5, 2024

luke-hill Jan 5, 2024

		require 'cucumber/ci_environment'

		describe 'remove_user_info_from_url' do

		require 'cucumber/ci_environment'

		describe 'Cucumber::CiEnvironment::VariableExpression.evaluate' do

Enabling rubo configs/12 20 23 #241

Are you sure you want to change the base?

Enabling rubo configs/12 20 23 #241

Conversation

calderete commented Dec 20, 2023

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

📋 Checklist:

mpkorstanje commented Dec 20, 2023

olleolleolle commented Dec 20, 2023

luke-hill left a 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

luke-hill left a 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

calderete Jan 11, 2024 • edited

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

Choose a reason for hiding this comment

calderete Jan 11, 2024 •

edited