Add support for listing top architecture violations (#104)

* Merge list_top_X_violations commands into a single list_top_violations

DRYer code.

* Add support for listing top architecture violations

Author: oboxodo

README.md

@@ -49,21 +49,16 @@ modify packs/from_pack/package.yml's list of dependencies and add packs/to_pack.
 
 This command will also sort the list and make it unique.
 
-## List the top dependency violations of packs/your_pack
-`bin/packs list_top_dependency_violations packs/your_pack`
+## List the top violations of a specific type for packs/your_pack.
+`bin/packs list_top_violations type [ packs/your_pack ]`
 
-Want to see who is depending on you? Not sure how your pack's code is being used in an unstated way
+Possible types are: dependency, privacy, architecture.
 
-You can use this command to list the top dependency violations.
+Want to see who is depending on you? Not sure how your pack's code is being used in an unstated way? You can use this command to list the top dependency violations.
 
-If no pack name is passed in, this will list out violations across all packs.
-
-## List the top privacy violations of packs/your_pack
-`bin/packs list_top_privacy_violations packs/your_pack`
-
-Want to create interfaces? Not sure how your pack's code is being used?
+Want to create interfaces? Not sure how your pack's code is being used? You can use this command to list the top privacy violations.
 
-You can use this command to list the top privacy violations.
+Want to focus on the big picture first? You can use this command to list the top architecture violations.
 
 If no pack name is passed in, this will list out violations across all packs.
 

lib/packs.rb

@@ -186,31 +186,18 @@ def self.move_to_parent!(
 
   sig do
     params(
+      type: String,
       pack_name: T.nilable(String),
       limit: Integer
     ).void
   end
-  def self.list_top_privacy_violations(
+  def self.list_top_violations(
+    type:,
     pack_name:,
     limit:
   )
-    Private::PackRelationshipAnalyzer.list_top_privacy_violations(
-      pack_name,
-      limit
-    )
-  end
-
-  sig do
-    params(
-      pack_name: T.nilable(String),
-      limit: Integer
-    ).void
-  end
-  def self.list_top_dependency_violations(
-    pack_name:,
-    limit:
-  )
-    Private::PackRelationshipAnalyzer.list_top_dependency_violations(
+    Private::PackRelationshipAnalyzer.list_top_violations(
+      type,
       pack_name,
       limit
     )

lib/packs/cli.rb

@@ -31,36 +31,31 @@ def add_dependency(from_pack, to_pack)
       exit_successfully
     end
 
-    desc 'list_top_dependency_violations packs/your_pack', 'List the top dependency violations of packs/your_pack'
+    POSIBLE_TYPES = T.let(%w[dependency privacy architecture], T::Array[String])
+    desc 'list_top_violations type [ packs/your_pack ]', 'List the top violations of a specific type for packs/your_pack.'
     long_desc <<~LONG_DESC
-      Want to see who is depending on you? Not sure how your pack's code is being used in an unstated way
+      Possible types are: #{POSIBLE_TYPES.join(', ')}.
 
-      You can use this command to list the top dependency violations.
+      Want to see who is depending on you? Not sure how your pack's code is being used in an unstated way? You can use this command to list the top dependency violations.
 
-      If no pack name is passed in, this will list out violations across all packs.
-    LONG_DESC
-    option :limit, type: :numeric, default: 10, aliases: :l, banner: 'Specify the limit of constants to analyze'
-    sig { params(pack_name: String).void }
-    def list_top_dependency_violations(pack_name)
-      Packs.list_top_dependency_violations(
-        pack_name: pack_name,
-        limit: options[:limit]
-      )
-      exit_successfully
-    end
-
-    desc 'list_top_privacy_violations packs/your_pack', 'List the top privacy violations of packs/your_pack'
-    long_desc <<~LONG_DESC
-      Want to create interfaces? Not sure how your pack's code is being used?
+      Want to create interfaces? Not sure how your pack's code is being used? You can use this command to list the top privacy violations.
 
-      You can use this command to list the top privacy violations.
+      Want to focus on the big picture first? You can use this command to list the top architecture violations.
 
       If no pack name is passed in, this will list out violations across all packs.
     LONG_DESC
     option :limit, type: :numeric, default: 10, aliases: :l, banner: 'Specify the limit of constants to analyze'
-    sig { params(pack_name: String).void }
-    def list_top_privacy_violations(pack_name)
-      Packs.list_top_privacy_violations(
+    sig do
+      params(
+        type: String,
+        pack_name: T.nilable(String)
+      ).void
+    end
+    def list_top_violations(type, pack_name = nil)
+      raise StandardError, "Invalid type #{type}. Possible types are: #{POSIBLE_TYPES.join(', ')}" unless POSIBLE_TYPES.include?(type)
+
+      Packs.list_top_violations(
+        type: type,
         pack_name: pack_name,
         limit: options[:limit]
       )

lib/packs/private/interactive_cli/use_cases/query.rb

@@ -21,7 +21,7 @@ def user_facing_name
           def perform!(prompt)
             selection = prompt.select('For one pack or all packs?', ['One pack', 'All packs'])
             if selection == 'All packs'
-              # Probably should just make `list_top_dependency_violations` take in an array of things
+              # Probably should just make `list_top_violations` take in an array of things
               # Better yet we might just want to replace these functions with `QueryPackwerk`
               selected_pack = nil
             else
@@ -30,19 +30,13 @@ def perform!(prompt)
 
             limit = prompt.ask('Specify the limit of constants to analyze', default: 10, convert: :integer)
 
-            selection = prompt.select('Are you interested in dependency or privacy violations?', %w[Dependency Privacy], default: 'Privacy')
+            selection = prompt.select('Are you interested in dependency, or privacy violations?', %w[Dependency Privacy Architecture], default: 'Privacy')
 
-            if selection == 'Dependency'
-              Packs.list_top_dependency_violations(
-                pack_name: selected_pack,
-                limit: limit
-              )
-            else
-              Packs.list_top_privacy_violations(
-                pack_name: selected_pack,
-                limit: limit
-              )
-            end
+            Packs.list_top_violations(
+              type: selection.downcase,
+              pack_name: selected_pack,
+              limit: limit
+            )
           end
         end
       end

lib/packs/private/pack_relationship_analyzer.rb

@@ -7,74 +7,12 @@ module PackRelationshipAnalyzer
 
       sig do
         params(
+          type: String,
           pack_name: T.nilable(String),
           limit: Integer
         ).void
       end
-      def self.list_top_privacy_violations(pack_name, limit)
-        all_packages = ParsePackwerk.all
-        if pack_name.nil?
-          to_package_names = all_packages.map(&:name)
-        else
-          pack_name = Private.clean_pack_name(pack_name)
-          package = all_packages.find { |p| p.name == pack_name }
-          if package.nil?
-            raise StandardError, "Can not find package with name #{pack_name}. Make sure the argument is of the form `packs/my_pack/`"
-          end
-
-          to_package_names = [pack_name]
-        end
-
-        violations_by_count = {}
-        total_pack_violation_count = 0
-
-        Logging.section('👋 Hi there') do
-          intro = Packs.config.user_event_logger.before_list_top_privacy_violations(pack_name, limit)
-          Logging.print_bold_green(intro)
-        end
-
-        # TODO: This is a copy of the implementation below. We may want to refactor out this implementation detail before making changes that apply to both.
-        all_packages.each do |client_package|
-          client_package.violations.select(&:privacy?).each do |violation|
-            next unless to_package_names.include?(violation.to_package_name)
-
-            if pack_name.nil?
-              violated_symbol = "#{violation.class_name} (#{violation.to_package_name})"
-            else
-              violated_symbol = violation.class_name
-            end
-            violations_by_count[violated_symbol] ||= {}
-            violations_by_count[violated_symbol][:total_count] ||= 0
-            violations_by_count[violated_symbol][:by_package] ||= {}
-            violations_by_count[violated_symbol][:by_package][client_package.name] ||= 0
-            violations_by_count[violated_symbol][:total_count] += violation.files.count
-            violations_by_count[violated_symbol][:by_package][client_package.name] += violation.files.count
-            total_pack_violation_count += violation.files.count
-          end
-        end
-
-        Logging.print("Total Count: #{total_pack_violation_count}")
-
-        sorted_violations = violations_by_count.sort_by { |violated_symbol, count_info| [-count_info[:total_count], violated_symbol] }
-        sorted_violations.first(limit).each do |violated_symbol, count_info|
-          percentage_of_total = (count_info[:total_count] * 100.0 / total_pack_violation_count).round(2)
-          Logging.print(violated_symbol)
-          Logging.print("  - Total Count: #{count_info[:total_count]} (#{percentage_of_total}% of total)")
-
-          Logging.print('  - By package:')
-          count_info[:by_package].sort_by { |client_package_name, count| [-count, client_package_name] }.each do |client_package_name, count|
-            Logging.print("    - #{client_package_name}: #{count}")
-          end
-        end
-      end
-
-      sig do
-        params(
-          pack_name: T.nilable(String),
-          limit: Integer
-        ).void
-      end
-      def self.list_top_dependency_violations(pack_name, limit)
+      def self.list_top_violations(type, pack_name, limit)
         all_packages = ParsePackwerk.all
 
         if pack_name.nil?
@@ -90,16 +28,15 @@ def self.list_top_dependency_violations(pack_name, limit)
         end
 
         Logging.section('👋 Hi there') do
-          intro = Packs.config.user_event_logger.before_list_top_dependency_violations(pack_name, limit)
+          intro = Packs.config.user_event_logger.before_list_top_violations(type, pack_name, limit)
           Logging.print_bold_green(intro)
         end
 
         violations_by_count = {}
         total_pack_violation_count = 0
 
-        # TODO: This is a copy of the implementation above. We may want to refactor out this implementation detail before making changes that apply to both.
         all_packages.each do |client_package|
-          client_package.violations.select(&:dependency?).each do |violation|
+          client_package.violations.select { _1.type == type }.each do |violation|
             next unless to_package_names.include?(violation.to_package_name)
 
             if pack_name.nil?

lib/packs/user_event_logger.rb

@@ -117,7 +117,7 @@ def on_create_public_directory_todo(pack_name)
         You can prevent other packs from using private API by using packwerk.
 
         Want to find how your private API is being used today?
-        Try running: `bin/packs list_top_privacy_violations #{pack_name}`
+        Try running: `bin/packs list_top_violations privacy #{pack_name}`
 
         Want to move something into this folder?
         Try running: `bin/packs make_public #{pack_name}/path/to/file.rb`
@@ -149,41 +149,20 @@ def on_create_readme_todo(pack_name)
       MSG
     end
 
-    sig { params(pack_name: T.nilable(String), limit: Integer).returns(String) }
-    def before_list_top_dependency_violations(pack_name, limit)
+    sig { params(type: String, pack_name: T.nilable(String), limit: Integer).returns(String) }
+    def before_list_top_violations(type, pack_name, limit)
       if pack_name.nil?
         <<~PACK_CONTENT
-          You are listing top #{limit} dependency violations for all packs. See #{documentation_link} for other utilities!
-          Pass in a limit to display more or less, e.g. `bin/packs list_top_dependency_violations #{pack_name} -l 1000`
+          You are listing top #{limit} #{type} violations for all packs. See #{documentation_link} for other utilities!
+          Pass in a limit to display more or less, e.g. `bin/packs list_top_violations #{type} #{pack_name} -l 1000`
 
           This script is intended to help you find which of YOUR pack's private classes, constants, or modules other packs are using the most.
           Anything not in pack_name/app/public is considered private API.
         PACK_CONTENT
       else
         <<~PACK_CONTENT
-          You are listing top #{limit} dependency violations for #{pack_name}. See #{documentation_link} for other utilities!
-          Pass in a limit to display more or less, e.g. `bin/packs list_top_dependency_violations #{pack_name} -l 1000`
-
-          This script is intended to help you find which of YOUR pack's private classes, constants, or modules other packs are using the most.
-          Anything not in #{pack_name}/app/public is considered private API.
-        PACK_CONTENT
-      end
-    end
-
-    sig { params(pack_name: T.nilable(String), limit: Integer).returns(String) }
-    def before_list_top_privacy_violations(pack_name, limit)
-      if pack_name.nil?
-        <<~PACK_CONTENT
-          You are listing top #{limit} privacy violations for all packs. See #{documentation_link} for other utilities!
-          Pass in a limit to display more or less, e.g. `bin/packs list_top_privacy_violations #{pack_name} -l 1000`
-
-          This script is intended to help you find which of YOUR pack's private classes, constants, or modules other packs are using the most.
-          Anything not in pack_name/app/public is considered private API.
-        PACK_CONTENT
-      else
-        <<~PACK_CONTENT
-          You are listing top #{limit} privacy violations for #{pack_name}. See #{documentation_link} for other utilities!
-          Pass in a limit to display more or less, e.g. `bin/packs list_top_privacy_violations #{pack_name} -l 1000`
+          You are listing top #{limit} #{type} violations for #{pack_name}. See #{documentation_link} for other utilities!
+          Pass in a limit to display more or less, e.g. `bin/packs list_top_violations #{type} #{pack_name} -l 1000`
 
           This script is intended to help you find which of YOUR pack's private classes, constants, or modules other packs are using the most.
           Anything not in #{pack_name}/app/public is considered private API.

spec/packs/private/cli_spec.rb

@@ -32,23 +32,36 @@ def expect_failure
     end
   end
 
-  describe '#list_top_dependency_violations' do
+  describe '#list_top_violations for dependency' do
     it 'lists the top dependency violations' do
-      expect(Packs).to receive(:list_top_dependency_violations).with(
+      expect(Packs).to receive(:list_top_violations).with(
+        type: 'dependency',
         pack_name: 'packs/your_pack',
         limit: 10
       )
-      described_class.start(['list_top_dependency_violations', 'packs/your_pack'])
+      described_class.start(['list_top_violations', 'dependency', 'packs/your_pack'])
     end
   end
 
-  describe '#list_top_privacy_violations' do
+  describe '#list_top_violations for privacy' do
     it 'lists the top privacy violations' do
-      expect(Packs).to receive(:list_top_privacy_violations).with(
+      expect(Packs).to receive(:list_top_violations).with(
+        type: 'privacy',
         pack_name: 'packs/your_pack',
         limit: 10
       )
-      described_class.start(['list_top_privacy_violations', 'packs/your_pack'])
+      described_class.start(['list_top_violations', 'privacy', 'packs/your_pack'])
+    end
+  end
+
+  describe '#list_top_violations for architecture' do
+    it 'lists the top architecture violations' do
+      expect(Packs).to receive(:list_top_violations).with(
+        type: 'architecture',
+        pack_name: 'packs/your_pack',
+        limit: 10
+      )
+      described_class.start(['list_top_violations', 'architecture', 'packs/your_pack'])
     end
   end