A `gem` action #1066

mike-burns · 2021-03-13T06:27:55Z

For too long we have suffered under the tyranny of Rails' mediocre gem
action! But Rails 6 took it a step too far: it no longer supports a
destroy generator.

So it's high time we make our own. In the process, encode the rules we
like to see: keep gems alphabetical, and use the group block syntax
instead of the :group argument.

When inserting a gem, we have to consider two cases: do we have a group
specification? If we don't have a group name, insert it alphabetically
before the first non-indented gem "..." declaration. If we do have a
group name, find the block for that group and insert it alphabetically.
But there's also the chance thar we are the first to add this group, in
which case we should add the group with the gem line at the end.

When removing a group, we again consider the groups case. If there is no
group specification, remove the first matching non-indented gem "..."
declaration. If we have a group spec, find the group block in the
Gemfile and remove it. If there is no such group block, do nothing.

Closes #915.

For too long we have [suffered] under the tyranny of Rails' mediocre `gem` action! But Rails 6 took it a step too far: it [no longer] supports a `destroy` generator. [suffered]: #915 [no longer]:#1061 (comment) So it's high time we make our own. In the process, encode the rules we like to see: keep gems alphabetical, and use the `group` block syntax instead of the `:group` argument. When inserting a gem, we have to consider two cases: do we have a group specification? If we don't have a group name, insert it alphabetically before the first non-indented `gem "..."` declaration. If we do have a group name, find the block for that group and insert it alphabetically. But there's also the chance thar we are the first to add this group, in which case we should add the group with the `gem` line at the end. When removing a group, we again consider the groups case. If there is no group specification, remove the first matching non-indented `gem "..."` declaration. If we have a group spec, find the group block in the Gemfile and remove it. If there is no such group block, do nothing. Closes #915.

mike-burns · 2021-03-13T06:29:07Z

lib/suspenders/actions.rb

+            positioning = final_group(existing_gemfile)
+
+            if positioning
+              insert_gemline(existing_gemfile, indentation: 2, positioning: positioning)


I think we need to also generate the group block here and below. Whoops!

lib/suspenders/actions.rb

> No, the "Flying Saucer Men" are the Flying Saucer Men-- Narzac! Galaxy > 66, the Evil Empire! What's wrong with you! Clavius, AEon Flux, "Utopia or Deuteranopia?" --- In other news, we were generating stylesheet stuff when `--api` was passed and this lead to errors. Not sure how this caused us to catch it, but we can fix that now.

jsilasbailey

I ran this version and generated a test project, looking much better! Just a little bit of a mix up with adding suspenders itself and the empty trailing development group and rack-timeout gem.

source "https://rubygems.org"

git_source(:github) do |repo_name|
  repo_name = "#{repo_name}/#{repo_name}" unless repo_name.include?("/")
  "https://github.com/#{repo_name}.git"
end

ruby "2.7.2"


gem "autoprefixer-rails"

gem "bootsnap", require: false
gem "bourbon", ">= 6.0.0"
gem "delayed_job_active_record"
gem "high_voltage"
gem "honeybadger"
gem "inline_svg"
gem "oj"
gem "pg"
gem "puma"
gem "rack-canonical-host"
gem "rack-mini-profiler", require: false
gem "rails", "~> 6.0.0"
gem "recipient_interceptor"
gem "sassc-rails"
gem "simple_form"
gem "skylight"
gem "spring"
gem "spring-watcher-listen", "~> 2.0.0"
gem "sprockets", "< 4"
gem "title"
gem "tzinfo-data", platforms: [:mingw, :x64_mingw, :mswin, :jruby]
gem "webpacker"

group :development do
  gem "listen"
  gem "rspec-rails", "~> 3.6"
  gem "shoulda-matchers"
  gem "spring-commands-rspec"
  gem "standard"
  gem "web-console"
end

group :development, :test do
  gem "awesome_print"
  gem "bullet"
  gem "bundler-audit", ">= 0.7.0", require: false
  gem "factory_bot_rails"
  gem "pry-byebug"
  gem "pry-rails"
end

group :test do
  gem "formulaic"
  gem "launchy"
  gem "simplecov", require: false
  gem "timecop"
  gem "webdrivers"
  gem "webmock"
end

gem "suspenders", group: [:development, :test]

group :development do

end
  gem "rack-timeout"

jsilasbailey · 2021-03-26T13:50:17Z

lib/suspenders/generators/app_generator.rb

@@ -143,11 +143,11 @@ def generate_default
      generate("suspenders:profiler")
      generate("suspenders:json")
      generate("suspenders:static")


Nice catch. Do we still want static site support when the api option is passed? Not necessary to address in this PR just thought of it because of the stylesheet issue.

Yeah probably not -- but I'm going to save that for another PR.

jsilasbailey · 2021-03-26T14:05:01Z

lib/suspenders/actions.rb

+
+      def gem_group(gemfile)
+        groups_re = groups.map { |group| ":#{group}" }.join("[, ]+")
+        data = /^group[ (]+#{groups_re}[ )]+do.+?^end/mo.match(gemfile)


Small thing: This works for the case when the groups are in the order specified (ie: :test, :development vs. :development, :test). Don't really think it's worth addressing since I don't know a simple way to do it with regex and it's simple enough to just make sure we are specifying the correct order in all of our calls to gem.

Right ... we could explode all the permutations with an | but it really doesn't seem worth it for just Suspenders.

jsilasbailey · 2021-03-26T14:18:13Z

lib/suspenders/actions.rb

+      end
+
+      def gemline
+        parts = [name, version].compact.map { |part| part.inspect }


Suggested change

parts = [name, version].compact.map { |part| part.inspect }

parts = [name, version].compact.map(&:inspect)

jsilasbailey · 2021-03-26T14:23:48Z

lib/suspenders/actions.rb

+        if groups.present?
+          positioning = gem_group(existing_gemfile)
+
+          if positioning
+            insert_gemline(existing_gemfile, indentation: 2, positioning: positioning)
+          else
+            positioning = final_group(existing_gemfile)
+
+            if positioning
+              insert_gemline(existing_gemfile, indentation: 2, positioning: positioning)
+            else
+              insert_gemline(existing_gemfile, indentation: 2, positioning: [0, existing_gemfile.bytes.size])
+            end
+          end
+        else
+          insert_gemline(existing_gemfile, indentation: 0, positioning: [0, existing_gemfile.bytes.size])
+        end


This is a deep enough nested conditional block it might benefit from some more method extractions for readability

jsilasbailey · 2021-03-26T14:27:35Z

lib/suspenders/actions.rb

+      end
+
+      def invoke!
+        existing_gemfile = IO.read(gemfile_path)


Is there a reason we need to pass existing_gemfile around to all the methods?

I was trying to avoid doing IO in the initializer. But maybe it's fine so long as I'm not, like, writing to disc in the initializer.

Yeah it's a pretty good reason and it's already this way so your choice on the matter. It would certainly help the arity of the private methods stay to <=2 but at the end of the day it's minor.

These are all good reasons. One option is to pass the IO object to the initializer, but not very practical. Maybe we could do something like this:

def existing_gemfile @existing_gemfile ||= IO.read(gemfile_path) end

Which incurs the IO cost only once and avoids duplication (at the expense of "where does this come from?").

jsilasbailey · 2021-03-26T19:48:57Z

It is certainly a better gem 💎

Reliably remove comments from Rails config files when generating Suspenders apps. With the parser gem, we're now removing comments by processing their surrounding code using AST (abstract syntax tree) processors. We were using a regex to do that, which is text-based and thereby subject to edge cases that only a proper AST solution can handle. [This pull request](#1044) made changes to some config files to fix the Action Mailer asset host. While working on merging that PR, the tests were not passing because they looked for an output that was missing in one of the changed config files: ```ruby config.action_mailer.asset_host = %q{"https://#{ENV\.fetch$"ASSET_HOST", ENV\.fetch\("APPLICATION_HOST"$\)}} ``` The PR code changes are kind of straightforward and that config option should have been present in the config file but it was not. The reason was that Suspenders was removing code comments with an overly permissive regex : (`/^.*#.*$/`), which removed every line containing a `#` character, no matter where they were in the line - so it was also removing the new Action Mailer config setting. That regex was also silently messing up with other config lines, for example: ```ruby config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } ``` As a quick fix to merge that PR, I've changed the regex to `/^\s*#.*$/` but it was not an ideal solution - which is what I'm striving for with this PR. With the old text-based regex solution, there were still nasty edge cases to deal with, and we may never know when they'd occur. Also, the previous regex did not work with inline comments. The final aesthetics of the config files could be improved in my opinion. This was how they looked like: ```ruby Rails.application.configure do config.cache_classes = false config.eager_load = false config.consider_all_requests_local = true if Rails.root.join('tmp', 'caching-dev.txt').exist? config.action_controller.perform_caching = true config.action_controller.enable_fragment_cache_logging = true config.cache_store = :memory_store config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } else config.action_controller.perform_caching = false config.cache_store = :null_store end config.active_storage.service = :local config.action_mailer.raise_delivery_errors = true config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true Bullet.rails_logger = true end # ... end ``` And with the newest solution they look like this: ```ruby Rails.application.configure do config.cache_classes = false config.eager_load = false config.consider_all_requests_local = true if Rails.root.join('tmp', 'caching-dev.txt').exist? config.action_controller.perform_caching = true config.action_controller.enable_fragment_cache_logging = true config.cache_store = :memory_store config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } else config.action_controller.perform_caching = false config.cache_store = :null_store end config.action_mailer.raise_delivery_errors = true config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true Bullet.rails_logger = true end # ... ``` Which preserves newlines where appropriate, so that's easier to read. This pull request uses the `parser` gem, and I feel like introducing that gem in the project may be a good move over the long term. For example, we need to finish up the [gem action](#1066) to sort `gem` declarations alphabetically when adding gems to the `Gemfile`, and the most reliable way to do that is with AST parsing. 1. Make the indentation of some templates match the Rails environment's config files'. This goes along with the aesthetics introduced in this PR. 2. Because of (1), I've also found a bug that was concatenaning two lines of codes in `config/environments/production.rb` with no blank line between them (in email_generator.rb), so I'm taking the opportunity to fix it 3. standardrb fixes after updating to the latest standardrb version

** First motivation Reliably remove comments from Rails config files when generating Suspenders apps. With the parser gem, we're now removing comments by processing their surrounding code using AST (abstract syntax tree) processors. We were using a regex to do that, which is text-based and thereby subject to edge cases that only a proper AST solution can handle. [This pull request](#1044) made changes to some config files to fix the Action Mailer asset host. While working on merging that PR, the tests were not passing because they looked for an output that was missing in one of the changed config files: config.action_mailer.asset_host = %q{"https://#{ENV\.fetch$"ASSET_HOST", ENV\.fetch\("APPLICATION_HOST"$\)}} The PR code changes are kind of straightforward and that config option should have been present in the config file but it was not. The reason was that Suspenders was removing code comments with an overly permissive regex : (`/^.*#.*$/`), which removed every line containing a `#` character, no matter where they were in the line - so it was also removing the new Action Mailer config setting. That regex was also silently messing up with other config lines, for example: config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } As a quick fix to merge that PR, I've changed the regex to `/^\s*#.*$/` but it was not an ideal solution - which is what I'm striving for with this PR. With the old text-based regex solution, there were still nasty edge cases to deal with, and we may never know when they'd occur. Also, the previous regex did not work with inline comments. ** Second motivation The final aesthetics of the config files could be improved in my opinion. This was how they looked like: Rails.application.configure do config.cache_classes = false config.eager_load = false config.consider_all_requests_local = true if Rails.root.join('tmp', 'caching-dev.txt').exist? config.action_controller.perform_caching = true config.action_controller.enable_fragment_cache_logging = true config.cache_store = :memory_store config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } else config.action_controller.perform_caching = false config.cache_store = :null_store end config.active_storage.service = :local config.action_mailer.raise_delivery_errors = true config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true Bullet.rails_logger = true end end And with the newest solution they look like this: Rails.application.configure do config.cache_classes = false config.eager_load = false config.consider_all_requests_local = true if Rails.root.join('tmp', 'caching-dev.txt').exist? config.action_controller.perform_caching = true config.action_controller.enable_fragment_cache_logging = true config.cache_store = :memory_store config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } else config.action_controller.perform_caching = false config.cache_store = :null_store end config.action_mailer.raise_delivery_errors = true config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true Bullet.rails_logger = true end end Which preserves newlines where appropriate, so that's easier to read. ** Third motivation This pull request uses the `parser` gem, and I feel like introducing that gem in the project may be a good move over the long term. For example, we need to finish up the [gem action](#1066) to sort `gem` declarations alphabetically when adding gems to the `Gemfile`, and the most reliable way to do that is with AST parsing. ** Other related changes 1. Make the indentation of some templates match the Rails environment's config files'. This goes along with the aesthetics introduced in this PR. 2. Because of (1), I've also found a bug that was concatenaning two lines of codes in `config/environments/production.rb` with no blank line between them (in email_generator.rb), so I'm taking the opportunity to fix it 3. standardrb fixes after updating to the latest standardrb version

** First motivation Reliably remove comments from Rails config files when generating Suspenders apps. With the parser gem, we're now removing comments by processing their surrounding code using AST (abstract syntax tree) processors. We were using a regex to do that, which is text-based and thereby subject to edge cases that only a proper AST solution can handle. [This pull request](#1044) made changes to some config files to fix the Action Mailer asset host. While working on merging that PR, the tests were not passing because they looked for an output that was missing in one of the changed config files: config.action_mailer.asset_host = %q{"https://#{ENV\.fetch$"ASSET_HOST", ENV\.fetch\("APPLICATION_HOST"$\)}} The PR code changes are kind of straightforward and that config option should have been present in the config file but it was not. The reason was that Suspenders was removing code comments with an overly permissive regex : (`/^.*#.*$/`), which removed every line containing a `#` character, no matter where they were in the line - so it was also removing the new Action Mailer config setting. That regex was also silently messing up with other config lines, for example: config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } As a quick fix to merge that PR, I've changed the regex to `/^\s*#.*$/` but it was not an ideal solution - which is what I'm striving for with this PR. With the old text-based regex solution, there were still nasty edge cases to deal with, and we may never know when they'd occur. Also, the previous regex did not work with inline comments. ** Second motivation The final aesthetics of the config files could be improved in my opinion. This was how they looked like: Rails.application.configure do config.cache_classes = false config.eager_load = false config.consider_all_requests_local = true if Rails.root.join('tmp', 'caching-dev.txt').exist? config.action_controller.perform_caching = true config.action_controller.enable_fragment_cache_logging = true config.cache_store = :memory_store config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } else config.action_controller.perform_caching = false config.cache_store = :null_store end config.active_storage.service = :local config.action_mailer.raise_delivery_errors = true config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true Bullet.rails_logger = true end end And with the newest solution they look like this: Rails.application.configure do config.cache_classes = false config.eager_load = false config.consider_all_requests_local = true if Rails.root.join('tmp', 'caching-dev.txt').exist? config.action_controller.perform_caching = true config.action_controller.enable_fragment_cache_logging = true config.cache_store = :memory_store config.public_file_server.headers = { 'Cache-Control' => "public, max-age=#{2.days.to_i}" } else config.action_controller.perform_caching = false config.cache_store = :null_store end config.action_mailer.raise_delivery_errors = true config.after_initialize do Bullet.enable = true Bullet.bullet_logger = true Bullet.rails_logger = true end end Which preserves newlines where appropriate, so that's easier to read. ** Third motivation This pull request uses the `parser` gem, and I feel like introducing that gem in the project may be a good move over the long term. For example, we need to finish up the [gem action](#1066) to sort `gem` declarations alphabetically when adding gems to the `Gemfile`, and the most reliable way to do that is with AST parsing. ** Other related changes 1. Make the indentation of some templates match the Rails environment's config files'. This goes along with the aesthetics introduced in this PR. 2. Because of (1), I've also found a bug that was concatenating two lines of codes in `config/environments/production.rb` with no blank line between them (in email_generator.rb), so I'm taking the opportunity to fix it 3. standardrb fixes after updating to the latest standardrb version

This commit does the bare minimum to support Rails 6.1.0. Because the `gem` command is no longer reversible, I opted to replace all instances with `append_file`. However, #1066 provides a more elegant solution. Closes #1060

This commit does the bare minimum to support Rails 6.1.6.1 Because the `gem` command is no longer reversible, I opted to replace all instances with `append_file`. However, #1066 provides a more elegant solution. Closes #1060

This commit does the bare minimum to support Rails 6.1.6.1 Because the `gem` command is no longer reversible, we decided to override the method by borrowing as much from the [existing method](https://github.com/rails/rails/blob/04972d9b9ef60796dc8f0917817b5392d61fcf09/railties/lib/rails/generators/actions.rb#L22). The main difference being that we call `append_file` instead of `gsub_file`, so that we can preserve reversibility. However, #1066 provides a more elegant solution. Closes #1060

stevepolitodesign · 2022-08-15T14:16:22Z

We might want to consider closing this now that #1104 has been merged.

stevepolitodesign · 2023-03-31T14:17:47Z

@mike-burns do you think we still need this since merging #1104 and #1106?

mike-burns · 2023-04-03T15:20:10Z

Not sure, I haven't kept up. Does the destroy generator work again? If the generator adds a gem, does the destroy remove it?

stevepolitodesign · 2023-04-19T11:53:59Z

Not sure, I haven't kept up. Does the destroy generator work again? If the generator adds a gem, does the destroy remove it?

@mike-burns yes, the destroy generator works in that it removes the gem from the Gemfile. You can test this by removing the gem method from lib/suspenders/actions.rb and running one of the generator tests, like spec/suspenders/generators/lint_generator_spec.rb:32. However, I know your solution aims to improve things like positioning and formatting too.

mike-burns mentioned this pull request Mar 13, 2021

Rails 6.1.1 #1061

Closed

mike-burns commented Mar 13, 2021

View reviewed changes

mike-burns requested a review from jsilasbailey March 13, 2021 06:29

mike-burns commented Mar 19, 2021

View reviewed changes

lib/suspenders/actions.rb Outdated Show resolved Hide resolved

mike-burns added 2 commits March 19, 2021 11:50

"Style", if you can call it that

1f8b121

jsilasbailey reviewed Mar 26, 2021

View reviewed changes

thiagoa mentioned this pull request Nov 20, 2021

Make removing comments from config files work reliably #1085

Merged

thiagoa force-pushed the main branch from 6ad77cd to 2ac46d3 Compare January 28, 2022 15:18

stevepolitodesign mentioned this pull request Aug 5, 2022

Support Rails 6.1.6.1 #1104

Merged

mike-burns closed this Apr 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A `gem` action #1066

A `gem` action #1066

mike-burns commented Mar 13, 2021

mike-burns Mar 13, 2021

jsilasbailey left a comment •

edited

Loading

jsilasbailey Mar 26, 2021

mike-burns Mar 26, 2021

jsilasbailey Mar 26, 2021

mike-burns Mar 26, 2021

jsilasbailey Mar 26, 2021

jsilasbailey Mar 26, 2021

jsilasbailey Mar 26, 2021

mike-burns Mar 26, 2021

jsilasbailey Mar 26, 2021

thiagoa Aug 13, 2021 •

edited

Loading

jsilasbailey commented Mar 26, 2021

stevepolitodesign commented Aug 15, 2022

stevepolitodesign commented Mar 31, 2023

mike-burns commented Apr 3, 2023

stevepolitodesign commented Apr 19, 2023

	parts = [name, version].compact.map { \|part\| part.inspect }
	parts = [name, version].compact.map(&:inspect)

A gem action #1066

A gem action #1066

Conversation

mike-burns commented Mar 13, 2021

Choose a reason for hiding this comment

jsilasbailey left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagoa Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

jsilasbailey commented Mar 26, 2021

stevepolitodesign commented Aug 15, 2022

stevepolitodesign commented Mar 31, 2023

mike-burns commented Apr 3, 2023

stevepolitodesign commented Apr 19, 2023

A `gem` action #1066

A `gem` action #1066

jsilasbailey left a comment •

edited

Loading

thiagoa Aug 13, 2021 •

edited

Loading