From 039e04c651dc751d36bf6c0e8fe51b12ceff9432 Mon Sep 17 00:00:00 2001 From: kg8m Date: Tue, 7 Feb 2023 14:49:58 +0900 Subject: [PATCH 1/7] Support `--frozen` option for routing annotations Signed-off-by: kg8m --- lib/annotate/annotate_routes.rb | 24 ++++++++++++++++++------ lib/tasks/annotate_routes.rake | 1 + 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/annotate/annotate_routes.rb b/lib/annotate/annotate_routes.rb index 75cc421ed..4e3e46f99 100644 --- a/lib/annotate/annotate_routes.rb +++ b/lib/annotate/annotate_routes.rb @@ -30,10 +30,18 @@ def do_annotations(options = {}) new_content = annotate_routes(HeaderGenerator.generate(options), content, header_position, options) new_text = new_content.join("\n") - if rewrite_contents(existing_text, new_text) - puts "#{routes_file} was annotated." + if options[:frozen] + if needs_rewrite_contents?(existing_text, new_text) + abort "annotate error. #{routes_file} needs to be updated, but annotate was run with `--frozen`." + else + puts "#{routes_file} was not changed." + end else - puts "#{routes_file} was not changed." + if rewrite_contents(existing_text, new_text) + puts "#{routes_file} was annotated." + else + puts "#{routes_file} was not changed." + end end else puts "#{routes_file} could not be found." @@ -83,14 +91,18 @@ def strip_on_removal(content, header_position) end def rewrite_contents(existing_text, new_text) - if existing_text == new_text - false - else + if needs_rewrite_contents?(existing_text, new_text) File.open(routes_file, 'wb') { |f| f.puts(new_text) } true + else + false end end + def needs_rewrite_contents?(existing_text, new_text) + existing_text != new_text + end + def annotate_routes(header, content, header_position, options = {}) magic_comments_map, content = Helpers.extract_magic_comments_from_array(content) if %w(before top).include?(options[:position_in_routes]) diff --git a/lib/tasks/annotate_routes.rake b/lib/tasks/annotate_routes.rake index ae6829337..b2832d443 100644 --- a/lib/tasks/annotate_routes.rake +++ b/lib/tasks/annotate_routes.rake @@ -14,6 +14,7 @@ task :annotate_routes => :environment do options[:position_in_routes] = Annotate::Helpers.fallback(ENV['position_in_routes'], ENV['position']) options[:ignore_routes] = Annotate::Helpers.fallback(ENV['ignore_routes'], nil) options[:require] = ENV['require'] ? ENV['require'].split(',') : [] + options[:frozen] = Annotate::Helpers.true?(ENV['frozen']) options[:wrapper_open] = Annotate::Helpers.fallback(ENV['wrapper_open'], ENV['wrapper']) options[:wrapper_close] = Annotate::Helpers.fallback(ENV['wrapper_close'], ENV['wrapper']) AnnotateRoutes.do_annotations(options) From a76da821228434e15dcf08f5aa5839b736b8cbe3 Mon Sep 17 00:00:00 2001 From: kg8m Date: Wed, 29 Mar 2023 17:42:33 +0900 Subject: [PATCH 2/7] Prefer `elsif` to nested `if`s Signed-off-by: kg8m --- lib/annotate/annotate_routes.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/annotate/annotate_routes.rb b/lib/annotate/annotate_routes.rb index 4e3e46f99..b608f621c 100644 --- a/lib/annotate/annotate_routes.rb +++ b/lib/annotate/annotate_routes.rb @@ -36,12 +36,10 @@ def do_annotations(options = {}) else puts "#{routes_file} was not changed." end + elsif rewrite_contents(existing_text, new_text) + puts "#{routes_file} was annotated." else - if rewrite_contents(existing_text, new_text) - puts "#{routes_file} was annotated." - else - puts "#{routes_file} was not changed." - end + puts "#{routes_file} was not changed." end else puts "#{routes_file} could not be found." From 86dc2c56039a5705a5e7f804b3a3917a4de257b2 Mon Sep 17 00:00:00 2001 From: kg8m Date: Fri, 31 Mar 2023 01:36:56 +0900 Subject: [PATCH 3/7] Add tests for `frozen` option of routing annotations Signed-off-by: kg8m --- spec/lib/annotate/annotate_routes_spec.rb | 65 +++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/spec/lib/annotate/annotate_routes_spec.rb b/spec/lib/annotate/annotate_routes_spec.rb index 805aec66c..2d27b7458 100644 --- a/spec/lib/annotate/annotate_routes_spec.rb +++ b/spec/lib/annotate/annotate_routes_spec.rb @@ -556,6 +556,71 @@ end end end + + describe 'frozen option' do + let :aborted_message do + "annotate error. #{ROUTE_FILE} needs to be updated, but annotate was run with `--frozen`." + end + + let :rake_routes_result do + <<-EOS + Prefix Verb URI Pattern Controller#Action + myaction1 GET /url1(.:format) mycontroller1#action + myaction2 POST /url2(.:format) mycontroller2#action + myaction3 DELETE|GET /url3(.:format) mycontroller3#action + EOS + end + + before :each do + expect(File).to receive(:exist?).with(ROUTE_FILE).and_return(true).once + expect(File).to receive(:read).with(ROUTE_FILE).and_return(route_file_content).once + + expect(AnnotateRoutes::HeaderGenerator).to receive(:`).with('rake routes').and_return(rake_routes_result).once + end + + context 'when annotation does not exists' do + let :route_file_content do + '' + end + + it 'aborts' do + expect { AnnotateRoutes.do_annotations(frozen: true) }.to raise_error SystemExit, aborted_message + end + end + + context 'when annotation exists but is not updated' do + let :route_file_content do + <<~EOS + # == Route Map + # + # Prefix Verb URI Pattern Controller#Action + # myaction2 POST /url2(.:format) mycontroller2#action + # myaction3 DELETE|GET /url3(.:format) mycontroller3#action + EOS + end + + it 'aborts' do + expect { AnnotateRoutes.do_annotations(frozen: true) }.to raise_error SystemExit, aborted_message + end + end + + context 'when annotation exists and is already updated' do + let :route_file_content do + <<~EOS + # == Route Map + # + # Prefix Verb URI Pattern Controller#Action + # myaction1 GET /url1(.:format) mycontroller1#action + # myaction2 POST /url2(.:format) mycontroller2#action + # myaction3 DELETE|GET /url3(.:format) mycontroller3#action + EOS + end + + it 'does NOT abort' do + expect { AnnotateRoutes.do_annotations(frozen: true) }.not_to raise_error + end + end + end end describe '.remove_annotations' do From 8777bce632e6104bcf593de4236609f6c74c069d Mon Sep 17 00:00:00 2001 From: Cuong Tran Date: Thu, 30 Mar 2023 14:57:48 -0700 Subject: [PATCH 4/7] Update annotate_routes.rb --- lib/annotate/annotate_routes.rb | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/lib/annotate/annotate_routes.rb b/lib/annotate/annotate_routes.rb index b608f621c..395061580 100644 --- a/lib/annotate/annotate_routes.rb +++ b/lib/annotate/annotate_routes.rb @@ -30,13 +30,7 @@ def do_annotations(options = {}) new_content = annotate_routes(HeaderGenerator.generate(options), content, header_position, options) new_text = new_content.join("\n") - if options[:frozen] - if needs_rewrite_contents?(existing_text, new_text) - abort "annotate error. #{routes_file} needs to be updated, but annotate was run with `--frozen`." - else - puts "#{routes_file} was not changed." - end - elsif rewrite_contents(existing_text, new_text) + if rewrite_contents(existing_text, new_text) puts "#{routes_file} was annotated." else puts "#{routes_file} was not changed." @@ -89,16 +83,14 @@ def strip_on_removal(content, header_position) end def rewrite_contents(existing_text, new_text) - if needs_rewrite_contents?(existing_text, new_text) + content_changed = (existing_text != new_text) + + if content_changed + abort "annotate error. #{routes_file} needs to be updated, but annotate was run with `--frozen`." if options[:frozen] File.open(routes_file, 'wb') { |f| f.puts(new_text) } - true - else - false end - end - - def needs_rewrite_contents?(existing_text, new_text) - existing_text != new_text + + content_changed end def annotate_routes(header, content, header_position, options = {}) From 5f7de08d8d463d883bf1f723bf6447eff1708546 Mon Sep 17 00:00:00 2001 From: Cuong Tran Date: Thu, 30 Mar 2023 15:02:59 -0700 Subject: [PATCH 5/7] Update annotate_routes.rb --- lib/annotate/annotate_routes.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/annotate/annotate_routes.rb b/lib/annotate/annotate_routes.rb index 395061580..208bdb3b9 100644 --- a/lib/annotate/annotate_routes.rb +++ b/lib/annotate/annotate_routes.rb @@ -29,8 +29,7 @@ def do_annotations(options = {}) content, header_position = Helpers.strip_annotations(existing_text) new_content = annotate_routes(HeaderGenerator.generate(options), content, header_position, options) new_text = new_content.join("\n") - - if rewrite_contents(existing_text, new_text) + if rewrite_contents(existing_text, new_text, options[:frozen]) puts "#{routes_file} was annotated." else puts "#{routes_file} was not changed." @@ -82,11 +81,11 @@ def strip_on_removal(content, header_position) content end - def rewrite_contents(existing_text, new_text) + def rewrite_contents(existing_text, new_text, frozen) content_changed = (existing_text != new_text) if content_changed - abort "annotate error. #{routes_file} needs to be updated, but annotate was run with `--frozen`." if options[:frozen] + abort "annotate error. #{routes_file} needs to be updated, but annotate was run with `--frozen`." if frozen File.open(routes_file, 'wb') { |f| f.puts(new_text) } end From 0f4bf0f1613f8ab791fa1a59e4355db52cd22192 Mon Sep 17 00:00:00 2001 From: Cuong Tran Date: Thu, 30 Mar 2023 15:06:59 -0700 Subject: [PATCH 6/7] Update annotate_routes.rb --- lib/annotate/annotate_routes.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/annotate/annotate_routes.rb b/lib/annotate/annotate_routes.rb index 208bdb3b9..b76016c8a 100644 --- a/lib/annotate/annotate_routes.rb +++ b/lib/annotate/annotate_routes.rb @@ -45,7 +45,7 @@ def remove_annotations(_options={}) content, header_position = Helpers.strip_annotations(existing_text) new_content = strip_on_removal(content, header_position) new_text = new_content.join("\n") - if rewrite_contents(existing_text, new_text) + if rewrite_contents(existing_text, new_text, _options[:frozen]) puts "Annotations were removed from #{routes_file}." else puts "#{routes_file} was not changed (Annotation did not exist)." From affa8e90d8127ba59cc2d8a8675734289158fffc Mon Sep 17 00:00:00 2001 From: Cuong Tran Date: Thu, 30 Mar 2023 15:10:06 -0700 Subject: [PATCH 7/7] Update annotate_routes.rb --- lib/annotate/annotate_routes.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/annotate/annotate_routes.rb b/lib/annotate/annotate_routes.rb index b76016c8a..c9a2218ac 100644 --- a/lib/annotate/annotate_routes.rb +++ b/lib/annotate/annotate_routes.rb @@ -39,13 +39,13 @@ def do_annotations(options = {}) end end - def remove_annotations(_options={}) + def remove_annotations(options={}) if routes_file_exist? existing_text = File.read(routes_file) content, header_position = Helpers.strip_annotations(existing_text) new_content = strip_on_removal(content, header_position) new_text = new_content.join("\n") - if rewrite_contents(existing_text, new_text, _options[:frozen]) + if rewrite_contents(existing_text, new_text, options[:frozen]) puts "Annotations were removed from #{routes_file}." else puts "#{routes_file} was not changed (Annotation did not exist)." @@ -85,10 +85,10 @@ def rewrite_contents(existing_text, new_text, frozen) content_changed = (existing_text != new_text) if content_changed - abort "annotate error. #{routes_file} needs to be updated, but annotate was run with `--frozen`." if frozen + abort "annotate error. #{routes_file} needs to be updated, but annotate was run with `--frozen`." if frozen File.open(routes_file, 'wb') { |f| f.puts(new_text) } end - + content_changed end