Adding Rubocop Gems and Removing Rootstrap Rubocop #391

TimoPeraza · 2023-07-21T23:26:23Z

Description:

Based on what we decide in this document. In this PR, we are removing rootstrap-rubocop, adding more rubocop gems and updating the current ones to help us to improve our code style.

Also in this PR we are fixing each linter that appear, after run the new rubocop configuration.

Notes:

Tasks:

Add each element in this format

Risk:

Preview:

santib · 2023-07-24T18:04:02Z

.rubocop.yml

+  - .rubocop-rs-config.yml
+  - .rubocop-excludes.yml


just curious, but why do you like having these 2 files? don't you think that excluding cops is also part of the "rs config"?

Yes, I know it's part of the rs config but I thought could be better if we have all the exclusions in a specific file because it is easier to visualize and you can see all the places where you are excluding a cop. In the other file, there are too many rules and information. But it was just my idea, if you don't like it I can remove it.

santib · 2023-07-24T18:41:32Z

spec/requests/api/v1/sessions/create_spec.rb

-    it 'returns the user' do
+    it 'returns the user id' do
      expect(json[:user][:id]).to eq(user.id)
+    end
+
+    it 'returns the user email' do
      expect(json[:user][:email]).to eq(user.email)
+    end
+
+    it 'returns the user username' do
      expect(json[:user][:username]).to eq(user.username)
+    end
+
+    it 'returns the user uid' do
      expect(json[:user][:uid]).to eq(user.uid)
+    end
+
+    it 'returns the user provider' do
      expect(json[:user][:provider]).to eq('email')
+    end
+
+    it 'returns the user first name' do
      expect(json[:user][:first_name]).to eq(user.first_name)
+    end
+
+    it 'returns the user last name' do
      expect(json[:user][:last_name]).to eq(user.last_name)
    end


maybe would be better to keep it as it was and add :aggregate_failures ?

https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#aggregate_failures-true-default

spec/requests/api/v1/users/create_spec.rb

sebastiancaraballo · 2023-07-24T18:49:01Z

.irbrc

@@ -1,3 +1,5 @@
+# frozen_string_literal: true


Is this still needed?

I think they are standardrb/standard#181 (comment)

sebastiancaraballo · 2023-07-24T18:51:25Z

spec/requests/api/v1/settings_spec.rb

      expect(response).to be_successful
    end

    it 'returns no need to update' do
-      subject
+      endpoint


I'd prefer keeping subject

Agree, at least for when we are just declaring it. We could also modify the ~~ImplicitSubject~~ NamedSubject cop. I do prefer naming the subject for most of other cases, but not sure if it's the best to enforce it always.

I had to change it because the NamedSubject cop. I don't have any preference about it, I would like to hear the opinion of @santib and @JulianPasquale

I also feel a bit strange naming the subject. Always felt subject is descriptive enough.
That said, I don't have an strong opinion either. I'm actually ok disabling this cop, doesn't sound too critical

I don't have a strong opinion either 😅 whatever the team prefers. Or we can just go with Rubocop default, or check what is the preferred style by Standard.rb 🤷‍♂️

spec/requests/api/v1/settings_spec.rb

santib · 2023-07-25T13:49:19Z

.rubocop-excludes.yml

+RSpec/VerifiedDoubleReference:
+  Exclude:
+    - spec/policies/admin/application_policy_spec.rb
+    - spec/policies/admin/page_policy_spec.rb
+    - spec/policies/application_policy_spec.rb


wouldn't it be better to fix them?

The idea was to fix all of them in another PR, the main goal of this one was just to set the rubocop configuration and fix the ones that appear after setting it

santib · 2023-07-25T13:50:42Z

.rubocop-excludes.yml

@@ -0,0 +1,46 @@
+Lint/AmbiguousBlockAssociation:


I'm a bit worried to have apps start with so many exceptions.. it feels like we are encouraging people to do it lightly. No need to do it right now, but maybe we can tackle it in the future. What do you think?

I think we should have a deeper discussion about the cops and what do they enforce 🤓

.rubocop-rs-config.yml

rodrieiz

nice work! some questions
Is the idea of changing "subject" to "policy," "endpoint," etc., suggested by RuboCop, or is it a separate change?
Is it possible to move the RuboCop files to a folder, or do they have to be in the project's root directory?

PerezIgnacio · 2023-08-01T18:58:56Z

.rubocop-excludes.yml

@@ -0,0 +1,46 @@
+Lint/AmbiguousBlockAssociation:
+  Exclude:
+    - spec/**/*


If this is due to the change method, another option could be to add it in rs-config instead:

Lint/AmbiguousBlockAssociation: AllowedMethods: change

Not sure if there's another similar case in tests.

PerezIgnacio · 2023-08-01T19:08:23Z

spec/requests/api/v1/settings_spec.rb

      expect(response).to be_successful
    end

    it 'returns no need to update' do
-      subject
+      endpoint


Agree, at least for when we are just declaring it. We could also modify the ~~ImplicitSubject~~ NamedSubject cop. I do prefer naming the subject for most of other cases, but not sure if it's the best to enforce it always.

TimoPeraza · 2023-08-03T20:20:12Z

nice work! some questions Is the idea of changing "subject" to "policy," "endpoint," etc., suggested by RuboCop, or is it a separate change? Is it possible to move the RuboCop files to a folder, or do they have to be in the project's root directory?

About the first point, It is because the NamedSubject cop, which forces you to name your subjects.
About the second one, I think most of the guys agree that the rubocop files should be in the root directory,

JulianPasquale · 2023-08-04T15:02:51Z

.rubocop.yml

 AllCops:
+  NewCops: enable


Using this config, new cops ("pending" cops) will be enabled when we upgrade the gem. The other option would be to use disable and manually enable the new ones.
https://docs.rubocop.org/rubocop/versioning.html#pending-cops

I'm ok keeping it as enable. I think that will help us to be aware of the new cops and understand them in a better way so we can decide if we really want it or not.

I would also like to hear the group opinion here 🙂

I think I'm good with the enable option, especially since it's a base app

JulianPasquale

🔥

sebastiancaraballo · 2023-08-07T18:36:53Z

spec/spec_helper.rb

@@ -30,7 +32,7 @@
  end
  config.include FactoryBot::Syntax::Methods

-  config.before :each do


Is this okay?

https://rspec.rubystyle.guide/#redundant-beforeeach

.rubocop.yml

db/migrate/20161011151353_devise_create_users.rb

spec/requests/api/v1/passwords/edit_spec.rb

spec/requests/api/v1/settings_spec.rb

spec/requests/api/v1/users/create_spec.rb

spec/requests/api/v1/users/show_spec.rb

mwolman · 2023-08-08T17:59:17Z

spec/spec_helper.rb

@@ -30,7 +32,7 @@
  end
  config.include FactoryBot::Syntax::Methods

-  config.before :each do


https://rspec.rubystyle.guide/#redundant-beforeeach

spec/requests/api/v1/users/update_spec.rb

mwolman · 2023-08-09T16:34:05Z

spec/requests/api/v1/settings_spec.rb

+    before do
+      create(:setting_version)
+    end


Suggested change

before do

create(:setting_version)

end

before { create(:setting_version) }

santib reviewed Jul 24, 2023

View reviewed changes

TimoPeraza requested a review from a team July 24, 2023 18:36

santib reviewed Jul 24, 2023

View reviewed changes

spec/requests/api/v1/users/create_spec.rb Show resolved Hide resolved

santib requested a review from a team July 24, 2023 18:43

sebastiancaraballo reviewed Jul 24, 2023

View reviewed changes

santib reviewed Jul 25, 2023

View reviewed changes

.rubocop-rs-config.yml Outdated Show resolved Hide resolved

rodrieiz reviewed Jul 25, 2023

View reviewed changes

PerezIgnacio reviewed Aug 1, 2023

View reviewed changes

mwolman mentioned this pull request Aug 3, 2023

Rubocop suggestions #392

Merged

JulianPasquale reviewed Aug 4, 2023

View reviewed changes

JulianPasquale approved these changes Aug 4, 2023

View reviewed changes

sebastiancaraballo approved these changes Aug 7, 2023

View reviewed changes

santib approved these changes Aug 7, 2023

View reviewed changes

TimoPeraza and others added 6 commits August 8, 2023 11:00

adding rubocops/removing rootstrap-rubocop

3f1570f

fixing rubocop issues

199f237

Upgrade rubocop version

e15ec3b

Remove unnecessary configurations and move exclusions to the todo file

b6bac44

Create custom .rubocop.yml file for db folder

3de93b1

Apply code review suggestions

ffd72b0

TimoPeraza force-pushed the update/rubocop branch 2 times, most recently from bcfef68 to 5025149 Compare August 8, 2023 14:09

mwolman reviewed Aug 8, 2023

View reviewed changes

TimoPeraza force-pushed the update/rubocop branch from 5025149 to deb8db3 Compare August 8, 2023 22:57

mwolman approved these changes Aug 9, 2023

View reviewed changes

mwolman requested review from rodrieiz and PerezIgnacio August 9, 2023 16:39

PerezIgnacio approved these changes Aug 9, 2023

View reviewed changes

fixing comments

84d63ad

TimoPeraza force-pushed the update/rubocop branch from deb8db3 to 84d63ad Compare August 10, 2023 17:15

TimoPeraza merged commit 0175877 into main Aug 10, 2023
4 checks passed

TimoPeraza deleted the update/rubocop branch August 10, 2023 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Rubocop Gems and Removing Rootstrap Rubocop #391

Adding Rubocop Gems and Removing Rootstrap Rubocop #391

TimoPeraza commented Jul 21, 2023 •

edited

Loading

santib Jul 24, 2023

TimoPeraza Jul 24, 2023

santib Jul 24, 2023

sebastiancaraballo Jul 24, 2023

santib Jul 25, 2023

sebastiancaraballo Jul 24, 2023

PerezIgnacio Aug 1, 2023 •

edited

Loading

TimoPeraza Aug 3, 2023

JulianPasquale Aug 4, 2023

santib Aug 4, 2023

santib Jul 25, 2023

TimoPeraza Aug 3, 2023

santib Jul 25, 2023

sebastiancaraballo Aug 2, 2023

rodrieiz left a comment

PerezIgnacio Aug 1, 2023

PerezIgnacio Aug 1, 2023 •

edited

Loading

TimoPeraza commented Aug 3, 2023

JulianPasquale Aug 4, 2023

santib Aug 7, 2023

JulianPasquale left a comment

sebastiancaraballo Aug 7, 2023

mwolman Aug 8, 2023

mwolman Aug 8, 2023

mwolman Aug 9, 2023

Adding Rubocop Gems and Removing Rootstrap Rubocop #391

Adding Rubocop Gems and Removing Rootstrap Rubocop #391

Conversation

TimoPeraza commented Jul 21, 2023 • edited Loading

Description:

Notes:

Tasks:

Risk:

Preview:

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

PerezIgnacio Aug 1, 2023 • edited Loading

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

rodrieiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PerezIgnacio Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

TimoPeraza commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulianPasquale 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

TimoPeraza commented Jul 21, 2023 •

edited

Loading

PerezIgnacio Aug 1, 2023 •

edited

Loading

PerezIgnacio Aug 1, 2023 •

edited

Loading