From 2115f48f18ad5b9d05e63d723cab42b4199f029a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eva=20Kadlecova=CC=81?= Date: Fri, 22 Feb 2019 13:26:04 +0100 Subject: [PATCH] Fix matching generic patterns --- Gemfile.lock | 4 +- codeowners-checker.gemspec | 8 +-- lib/codeowners/checker.rb | 2 +- lib/codeowners/checker/group/pattern.rb | 10 ++++ lib/codeowners/cli/main.rb | 6 +- spec/codeowners/checker/group/pattern_spec.rb | 58 +++++++++++++++++++ spec/codeowners/checker_spec.rb | 45 ++++++++++---- 7 files changed, 113 insertions(+), 20 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 760f174..f33ddae 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -16,8 +16,8 @@ GEM fuzzy_match (2.1.0) git (1.5.0) jaro_winkler (1.5.1) - method_source (0.9.2) json (2.1.0) + method_source (0.9.2) parallel (1.12.1) parser (2.5.3.0) ast (~> 2.4.0) @@ -72,7 +72,7 @@ DEPENDENCIES rspec (~> 3.0) rubocop (~> 0.61.1) rubocop-rspec (~> 1.30) - simplecov + simplecov (~> 0.16.1) BUNDLED WITH 1.17.3 diff --git a/codeowners-checker.gemspec b/codeowners-checker.gemspec index 9667b7c..2d4c69c 100644 --- a/codeowners-checker.gemspec +++ b/codeowners-checker.gemspec @@ -7,7 +7,7 @@ require 'codeowners/checker/version' Gem::Specification.new do |spec| spec.name = 'codeowners-checker' spec.version = Codeowners::Checker::VERSION - spec.authors = ['Jônatas Davi Paganini', 'Eva Kadlecova', 'Michal Papis'] + spec.authors = ['Jônatas Davi Paganini', 'Eva Kadlecová', 'Michal Papis'] spec.email = ['bootcamp_team@toptal.com'] spec.summary = 'Check consistency of Github CODEOWNERS and git changes.' @@ -24,11 +24,11 @@ Gem::Specification.new do |spec| spec.add_dependency 'git', '~> 1.5' spec.add_dependency 'thor', '~> 0.20.3' spec.add_development_dependency 'bundler', '~> 1.16' - spec.add_development_dependency 'simplecov' + spec.add_development_dependency 'pry', '~> 0.12.2' spec.add_development_dependency 'rake', '~> 10.0' + spec.add_development_dependency 'rb-readline', '~> 0.5.5' spec.add_development_dependency 'rspec', '~> 3.0' spec.add_development_dependency 'rubocop', '~> 0.61.1' spec.add_development_dependency 'rubocop-rspec', '~> 1.30' - spec.add_development_dependency 'pry', '~> 0.12.2' - spec.add_development_dependency 'rb-readline', '~> 0.5.5' + spec.add_development_dependency 'simplecov', '~> 0.16.1' end diff --git a/lib/codeowners/checker.rb b/lib/codeowners/checker.rb index d6c063f..986627d 100644 --- a/lib/codeowners/checker.rb +++ b/lib/codeowners/checker.rb @@ -89,7 +89,7 @@ def defined_owner?(file) codeowners.find do |line| next unless line.pattern? - return true if file == line.pattern + return true if line.match_file?(file) end @when_new_file&.call(file) if @when_new_file diff --git a/lib/codeowners/checker/group/pattern.rb b/lib/codeowners/checker/group/pattern.rb index e694a4c..432db2c 100644 --- a/lib/codeowners/checker/group/pattern.rb +++ b/lib/codeowners/checker/group/pattern.rb @@ -27,9 +27,19 @@ def parse(line) @pattern, *@owners = line.split(/\s+/) end + def match_file?(file) + regex.match(file) + end + def to_s [@pattern, @owners].join(' ') end + + private + + def regex + Regexp.new(pattern.gsub(%r{/\*\*}, '(/[^/]+)+').gsub(/\*/, '[^/]+')) + end end end end diff --git a/lib/codeowners/cli/main.rb b/lib/codeowners/cli/main.rb index 489f095..0741c26 100644 --- a/lib/codeowners/cli/main.rb +++ b/lib/codeowners/cli/main.rb @@ -47,7 +47,7 @@ def write_codeowners end def suggest_add_to_codeowners(file) - return unless yes?("File added: #{file.inspect}. Add owner to CODEOWNERS?") + return unless yes?("File added: #{file.inspect}. Add owner to the CODEOWNERS file?") owner = ask('File owner(s): ') new_line = create_new_pattern(file, owner) @@ -73,7 +73,7 @@ def add_pattern(pattern, subgroups) return if insert_pattern_into_subgroup(pattern, subgroups) == true end - @checker.main_group.add(pattern) if yes?('Add to the end of the codeowners file?') + @checker.main_group.add(pattern) if yes?('Add to the end of the CODEOWNERS file?') end def insert_pattern_into_subgroup(pattern, subgroups) @@ -120,7 +120,7 @@ def apply_suggestion(line, suggestion) def make_suggestion(suggestion) ask(<<~QUESTION, limited_to: %w[y i e d]) - Replace with: #{suggestion}? + Replace with: #{suggestion.inspect}? (y) yes (i) ignore (e) edit the pattern diff --git a/spec/codeowners/checker/group/pattern_spec.rb b/spec/codeowners/checker/group/pattern_spec.rb index 96b3e3f..01719cf 100644 --- a/spec/codeowners/checker/group/pattern_spec.rb +++ b/spec/codeowners/checker/group/pattern_spec.rb @@ -3,6 +3,64 @@ require 'codeowners/checker/group/pattern' RSpec.describe Codeowners::Checker::Group::Pattern do + describe '#owner' do + subject { described_class.build(line) } + + let(:line) { 'pattern @owner @owner1 @owner2' } + + it 'returns the first owner' do + expect(subject.owner).to eq('@owner') + end + end + + describe '#match_file?' do + subject { described_class.build(line) } + + { + 'directory/* @owner @owner2' => { + 'file.rb' => false, + 'directory/file.rb' => true, + 'directory/subdirectory/file.rb' => false + }, + '* @owner' => { + 'file.rb' => true, + 'directory/file.rb' => true, + 'directory/subdirectory/file.rb' => true + }, + 'dir/dir1/* @owner' => { + 'file.rb' => false, + 'dir/file.rb' => false, + 'dir/dir1/file.rb' => true, + 'dir/dir1/dir2/file.rb' => false + }, + 'dir/dir1/file.rb @owner' => { + 'file.rb' => false, + 'dir/file.rb' => false, + 'dir/dir1/file.rb' => true, + 'dir/dir1/file1.rb' => false, + 'dir/dir1/dir2/file.rb' => false + }, + '*.js @owner' => { + 'file.rb' => false, + 'dir/file.js' => true, + 'dir/dir1/file.rb' => false, + 'dir/dir1/file1.js' => true, + 'dir/dir1/dir2/file.js' => true + } + }.each do |content, tests| + context "when the line is #{content.inspect}" do + let(:line) { content } + tests.each do |file, result| + if result + it { is_expected.to be_match_file(file) } + else + it { is_expected.not_to be_match_file(file) } + end + end + end + end + end + describe '#to_s' do subject { described_class.build(line) } diff --git a/spec/codeowners/checker_spec.rb b/spec/codeowners/checker_spec.rb index 20768aa..554698d 100644 --- a/spec/codeowners/checker_spec.rb +++ b/spec/codeowners/checker_spec.rb @@ -102,21 +102,46 @@ def setup_git_for_project end context 'when introducing a new file in the git tree' do - before do - on_project_folder do - filename = 'lib/new_file.rb' - File.open(filename, 'w+') do |file| - file.puts '# add some ruby code here' + context 'when the file is not in the CODEOWNERS' do + before do + on_project_folder do + filename = 'lib/new_file.rb' + File.open(filename, 'w+') do |file| + file.puts '# add some ruby code here' + end + git.add filename + git.commit('New ruby file on lib') end - git.add filename - git.commit('New ruby file on lib') + end + + let(:from) { 'HEAD~1' } + + it 'fails if the file is not referenced in .github/CODEOWNERS' do + expect(subject).to eq(missing_ref: ['lib/new_file.rb'], useless_pattern: []) end end - let(:from) { 'HEAD~1' } + context 'when the files are not in the CODEOWNERS but generic patterns are' do + before do + on_project_folder do + filename = 'lib/billing/new_file.rb' + filename2 = 'app/file.js' + Dir.mkdir('app') + File.open(filename, 'w+') { |file| file.puts '# add some ruby code here' } + File.open(filename2, 'w+') { |file| file.puts '# add some ruby code here' } + File.open('.github/CODEOWNERS', 'a+') { |f| f.puts '*.js @owner3' } + + git.add filename + git.add filename2 + git.commit('New files') + end + end + + let(:from) { 'HEAD~1' } - it 'fails if the file is not referenced in .github/CODEOWNERS' do - expect(subject).to eq(missing_ref: ['lib/new_file.rb'], useless_pattern: []) + it 'does not list the files as missing reference' do + expect(subject).to eq(missing_ref: [], useless_pattern: []) + end end end