Copy c5-conventions/rubocop.yml into the new app #148

bunnymatic · 2020-01-23T04:18:40Z

Problem

Our connection to the rubocop.yml file in https://github.com/carbonfive/c5-conventions seems fragile.

Updates to that repo will affect all consumers out in the world (who we don't know) and may cause issues with their app. We've already seen it affect 1 client project in house.

Solution

By default, pull the rubocop.yml from c5-conventions at the time the
app is constructed and pin it. This will disconnect the constructed app
from the possibly dynamic rubocop file.

Additionally, we add some commentary to the top of the file to indcate
the SHA which was the original pull and a note to suggest upgrading at
the developers' discretion.

Changes

by default, fetch the rubocop file from github and write it to the
newly created target app
add rspec
add a test for this new functionality (the pull of the rubocop from GitHub)
make the default rake task run rubocop and rspec

Fixes #146

bunnymatic · 2020-01-23T06:58:56Z

lib/raygun/raygun.rb

+        raise "#{command} failed with status #{$CHILD_STATUS.exitstatus}."
+      end
+
+      output
    end


this was a combo of running rubocop -a and adding the return results to the function which i wanted to grab the SHA from the c5-conventions repo.

as it turns out, this didn't actually work. rolled it back to just return the output which i needed for the git ls-remote.

If rubocop corrects this again, we may need to either suppress the rule or figure out why it's not working.

mattbrictson

Looks good! After talking with @christiannelson I think we need to have the guard clause to ensure this is done only for the C5 raygun-rails template. Otherwise I have a couple Ruby-isms to suggest; see comments below.

mattbrictson · 2020-01-23T17:29:12Z

lib/raygun/raygun.rb

+          ''
+        ].join("\n")
+        rubocop_contents = URI.open(rubocop_file)
+        write_file("#{@app_dir}/.rubocop.yml", prefix + rubocop_contents.string)


I like using Ruby heredoc for multiline strings like the prefix:

Suggested change

write_file("#{@app_dir}/.rubocop.yml", prefix + rubocop_contents.string)

write_file("#{@app_dir}/.rubocop.yml", <<~PREFIX + rubocop_contents.string)

# Sourced from #{C5_CONVENTIONS_REPO} @ #{sha}"

#

# If you make changes to this file, consider opening a PR to backport them to the c5-conventions repo:

# https://github.com/#{C5_CONVENTIONS_REPO}/blob/master/rubocop/rubocop.yml

PREFIX

Or taking it a step further:

IO.write("#{@app_dir}/.rubocop.yml", <<~RUBOCOP_YML) # Sourced from #{C5_CONVENTIONS_REPO} @ #{sha}" # # If you make changes to this file, consider opening a PR to backport them to the c5-conventions repo: # https://github.com/#{C5_CONVENTIONS_REPO}/blob/master/rubocop/rubocop.yml #{rubocop_contents.string} RUBOCOP_YML

mattbrictson · 2020-01-23T17:30:13Z

lib/raygun/raygun.rb

+    def write_file(filename, contents)
+      fp = File.open(filename, 'w')
+      fp.write(contents)
+      fp.close
+    end


I think this is equivalent to Ruby's:

IO.write(filename, contents)

https://ruby-doc.org/core-2.7.0/IO.html#method-c-write

mattbrictson · 2020-01-23T17:34:44Z

lib/raygun/raygun.rb

@@ -105,6 +107,8 @@ def copy_prototype

      FileUtils.mv         dirs_to_move, app_dir
      FileUtils.remove_dir extraneous_dir
+
+      fetch_rubocop_file


Could we guard this so we only do it for C5 repos? That way if raygun is used with a third-party template we are not potentially clobbering their rubocop config.

Suggested change

fetch_rubocop_file

fetch_rubocop_file if @prototype_repo == CARBONFIVE_REPO

Sort of like we do here:

raygun/lib/raygun/raygun.rb

Lines 173 to 179 in 03c07eb

def print_next_steps

if @prototype_repo == CARBONFIVE_REPO

print_next_steps_carbon_five

else

print_next_steps_for_custom_repo

end

end

bunnymatic

Addressed PR comments
Added rspec and set the default rake task to spec
Added a test for the fetch_rubocop_repo function

bunnymatic · 2020-01-24T06:56:26Z

lib/raygun/raygun.rb

+        raise "#{command} failed with status #{$CHILD_STATUS.exitstatus}."
+      end
+
+      output
    end


as it turns out, this didn't actually work. rolled it back to just return the output which i needed for the git ls-remote.

If rubocop corrects this again, we may need to either suppress the rule or figure out why it's not working.

mattbrictson

Looks good! One minor suggestion below. I am going to test this out locally later today.

mattbrictson · 2020-01-24T21:42:49Z

Rakefile

+rescue LoadError
+  # no rspec available
+end


I think we can remove this rescue because we have the rspec dependency listed in the gemspec. If a developer forgets to use bundle exec that is a legitimate mistake and they should see an exception.

mattbrictson

I tested this locally and ran into two issues.

The first is minor: the generated .rubocop.yml has this header that is misleading and not very useful.

# Configuration hierarchy:
#
# 1. Rubocop defaults
# 2. Carbon Five defaults (this file)
# 3. Project overrides
#
# See http://rubocop.readthedocs.io/en/latest/configuration/#inheriting-configuration-from-a-remote-url for details.
#

It would be nice to strip this out. Or maybe update c5-conventions to just get rid of it altogether? In any case, it's not a blocker for this PR.

The bigger issue is that the generated project results in a broken RuboCop config:

Running RuboCop...
Error: unrecognized cop Rails/ApplicationRecord found in .rubocop.yml, unrecognized cop Rails/HasAndBelongsToMany found in .rubocop.yml, unrecognized cop Rails/RakeEnvironment found in .rubocop.yml
RuboCop failed!

This because the c5-conventions file specifies Rails/* cops but does not require rubocop-rails! I think we need to fix this upstream before we can merge this PR. What do you think?

bunnymatic · 2020-01-25T03:06:26Z

I already fixed that. I think we just need to coordinate the merge with this carbonfive/c5-conventions#10.

bunnymatic · 2020-01-25T17:07:38Z

As for the thing about this comment in the rubocop file

# Configuration hierarchy:
#
# 1. Rubocop defaults
# 2. Carbon Five default
...

I agree with the fact that we should not include it. I read it a couple times and wondered if it even belongs where it is. If apps are actually pulling this in remotely, it is unlikely that anyone is looking at it so I wonder about the value of those comments. So I'm inclined to update comments in the C5 conventions repo so they make more sense and then they wouldn't have to be trimmed during this copy. And they might provide some valuable information.

mattbrictson

Thanks! This all makes sense. I assume you will merge carbonfive/c5-conventions#10 first?

mattbrictson · 2020-01-27T18:42:43Z

As for the thing about this comment in the rubocop file
...
I'm inclined to update comments in the C5 conventions repo so they make more sense and then they wouldn't have to be trimmed during this copy. And they might provide some valuable information.

Logged a ticket for this: carbonfive/c5-conventions#12

-------- Our connection to the rubocop.yml file in https://github.com/carbonfive/c5-conventions seems fragile. Updates to that repo will affect all consumers out in the world (who we don't know) and may cause issues with their app. We've already seen it affect 1 client project in house. Solution --------- By default, pull the `rubocop.yml` from `c5-conventions` at the time the app is constructed and pin it. This will disconnect the constructed app from the possibly dynamic rubocop file. Additionally, we add some commentary to the top of the file to indcate the SHA which was the original pull and a note to suggest upgrading at the developers' discretion. Changes ------- * by default, fetch the rubocop file from github and write it to the newly created target app * add rspec * add a test for this new functionality (the pull of the rubocop from GitHub) Fixes #146

bunnymatic force-pushed the features/include-c5-conventions-rubocop-file-instead-of-live-link branch 3 times, most recently from 6bbda03 to ae0f2f4 Compare January 23, 2020 06:59

bunnymatic commented Jan 23, 2020

View reviewed changes

mattbrictson suggested changes Jan 23, 2020

View reviewed changes

bunnymatic mentioned this pull request Jan 24, 2020

Require ruby 2.4 or greater #151

Merged

bunnymatic commented Jan 24, 2020

View reviewed changes

mattbrictson reviewed Jan 24, 2020

View reviewed changes

mattbrictson suggested changes Jan 25, 2020

View reviewed changes

bunnymatic force-pushed the features/include-c5-conventions-rubocop-file-instead-of-live-link branch 2 times, most recently from 03f8c3d to cc37fde Compare January 25, 2020 17:04

mattbrictson approved these changes Jan 27, 2020

View reviewed changes

mattbrictson mentioned this pull request Jan 27, 2020

Remove unnecessary rubocop.yml header comment carbonfive/c5-conventions#12

Closed

bunnymatic force-pushed the features/include-c5-conventions-rubocop-file-instead-of-live-link branch from cc37fde to 39b54e9 Compare January 29, 2020 19:06

bunnymatic force-pushed the features/include-c5-conventions-rubocop-file-instead-of-live-link branch from 5deb520 to 860eb23 Compare January 29, 2020 19:09

bunnymatic merged commit dbff3fd into master Jan 29, 2020

bunnymatic deleted the features/include-c5-conventions-rubocop-file-instead-of-live-link branch January 29, 2020 19:10

mattbrictson mentioned this pull request Jan 29, 2020

Set up RuboCop for this repo #149

Closed

mattbrictson mentioned this pull request Mar 5, 2020

Include C5 conventions .rubocop_rspec.yml config in generated projects #156

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy c5-conventions/rubocop.yml into the new app #148

Copy c5-conventions/rubocop.yml into the new app #148

bunnymatic commented Jan 23, 2020 •

edited

Loading

bunnymatic Jan 23, 2020

bunnymatic Jan 24, 2020

mattbrictson left a comment

mattbrictson Jan 23, 2020

mattbrictson Jan 23, 2020

mattbrictson Jan 23, 2020

mattbrictson Jan 23, 2020

bunnymatic left a comment

bunnymatic Jan 24, 2020

mattbrictson left a comment

mattbrictson Jan 24, 2020

mattbrictson left a comment

bunnymatic commented Jan 25, 2020

bunnymatic commented Jan 25, 2020

mattbrictson left a comment

mattbrictson commented Jan 27, 2020

-        write_file("#{@app_dir}/.rubocop.yml", prefix + rubocop_contents.string)
+        write_file("#{@app_dir}/.rubocop.yml", <<~PREFIX + rubocop_contents.string)
+          # Sourced from #{C5_CONVENTIONS_REPO} @ #{sha}"
+          #
+          # If you make changes to this file, consider opening a PR to backport them to the c5-conventions repo:
+          # https://github.com/#{C5_CONVENTIONS_REPO}/blob/master/rubocop/rubocop.yml
+        PREFIX

	fetch_rubocop_file
	fetch_rubocop_file if @prototype_repo == CARBONFIVE_REPO

	def print_next_steps
	if @prototype_repo == CARBONFIVE_REPO
	print_next_steps_carbon_five
	else
	print_next_steps_for_custom_repo
	end
	end

Copy c5-conventions/rubocop.yml into the new app #148

Copy c5-conventions/rubocop.yml into the new app #148

Conversation

bunnymatic commented Jan 23, 2020 • edited Loading

Problem

Solution

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbrictson 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

bunnymatic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbrictson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbrictson left a comment

Choose a reason for hiding this comment

bunnymatic commented Jan 25, 2020

bunnymatic commented Jan 25, 2020

mattbrictson left a comment

Choose a reason for hiding this comment

mattbrictson commented Jan 27, 2020

bunnymatic commented Jan 23, 2020 •

edited

Loading