Aw/ts/wip add modularization library to format offenses

README.md

@@ -43,8 +43,8 @@ class MyFormatter
   extend T::Sig
   include DangerPackwerk::Check::OffensesFormatter
   # Packwerk::ReferenceOffense: https://github.com/Shopify/packwerk/blob/main/lib/packwerk/reference_offense.rb
-  sig { override.params(offenses: T::Array[Packwerk::ReferenceOffense], repo_link: String, org_name: String).returns(String) }
-  def format_offenses(offenses, repo_link, org_name)
+  sig { override.params(offenses: T::Array[Packwerk::ReferenceOffense], repo_link: String, org_name: String, modularization_library: String ).returns(String) }
+  def format_offenses(offenses, repo_link, org_name, modularization_library)
     # your logic here
   end
 end

lib/danger-packwerk/check/default_formatter.rb

@@ -21,10 +21,11 @@ def initialize(custom_help_message: nil)
         override.params(
           offenses: T::Array[Packwerk::ReferenceOffense],
           repo_link: String,
-          org_name: String
+          org_name: String,
+          modularization_library: String
         ).returns(String)
       end
-      def format_offenses(offenses, repo_link, org_name)
+      def format_offenses(offenses, repo_link, org_name, modularization_library: 'packwerk')
         reference_offense = T.must(offenses.first)
         violation_types = offenses.map(&:violation_type)
         referencing_file = reference_offense.reference.relative_path
@@ -38,7 +39,11 @@ def format_offenses(offenses, repo_link, org_name)
         constant_source_package = T.must(ParsePackwerk.all.find { |p| p.name == constant_source_package_name })
         constant_source_package_ownership_info = Private::OwnershipInformation.for_package(constant_source_package, org_name)
 
-        disclaimer = 'Before you run `bin/packwerk update-todo`, check out these quick suggestions:'
+        if modularization_library == 'packwerk'
+          disclaimer = 'Before you run `bin/packwerk update-todo`, check out these quick suggestions:'
+        elsif modularization_library == 'pks'
+          disclaimer = 'Before you run `bin/pks update`, check out these quick suggestions:'
+        end
         referencing_code_in_right_pack = "- Does the code you are writing live in the right pack?\n  - If not, try `bin/packs move packs/destination_pack #{referencing_file}`"
         referenced_code_in_right_pack = "- Does #{constant_name} live in the right pack?\n  - If not, try `bin/packs move packs/destination_pack #{constant_location}`"
         dependency_violation_message = "- Do we actually want to depend on #{constant_source_package_name}?\n  - If so, try `bin/packs add_dependency #{referencing_file_pack} #{constant_source_package_name}`\n  - If not, what can we change about the design so we do not have to depend on #{constant_source_package_name}?"

lib/danger-packwerk/check/offenses_formatter.rb

@@ -14,10 +14,11 @@ module OffensesFormatter
         abstract.params(
           offenses: T::Array[Packwerk::ReferenceOffense],
           repo_link: String,
-          org_name: String
+          org_name: String,
+          modularization_library: String
         ).returns(String)
       end
-      def format_offenses(offenses, repo_link, org_name); end
+      def format_offenses(offenses, repo_link, org_name, modularization_library: 'packwerk'); end
     end
   end
 end

lib/danger-packwerk/danger_package_todo_yml_changes.rb

@@ -30,15 +30,17 @@ class DangerPackageTodoYmlChanges < Danger::Plugin
         before_comment: BeforeComment,
         max_comments: Integer,
         violation_types: T::Array[String],
-        root_path: T.nilable(String)
+        root_path: T.nilable(String),
+        modularization_library: String
       ).void
     end
     def check(
       offenses_formatter: nil,
       before_comment: DEFAULT_BEFORE_COMMENT,
       max_comments: DEFAULT_MAX_COMMENTS,
       violation_types: DEFAULT_VIOLATION_TYPES,
-      root_path: nil
+      root_path: nil,
+      modularization_library: 'packwerk'
     )
       offenses_formatter ||= Update::DefaultFormatter.new
       repo_link = github.pr_json[:base][:repo][:html_url]
@@ -62,7 +64,7 @@ def check(
         location = T.must(violations.first).file_location
 
         markdown(
-          offenses_formatter.format_offenses(violations, repo_link, org_name),
+          offenses_formatter.format_offenses(violations, repo_link, org_name, modularization_library: modularization_library),
           line: location.line_number,
           file: git_filesystem.convert_to_filesystem(location.file)
         )

lib/danger-packwerk/danger_packwerk.rb

@@ -45,7 +45,8 @@ class CommentGroupingStrategy < ::T::Enum
         on_failure: OnFailure,
         violation_types: T::Array[String],
         grouping_strategy: CommentGroupingStrategy,
-        root_path: T.nilable(String)
+        root_path: T.nilable(String),
+        modularization_library: String
       ).void
     end
     def check(
@@ -56,7 +57,8 @@ def check(
       on_failure: DEFAULT_ON_FAILURE,
       violation_types: DEFAULT_VIOLATION_TYPES,
       grouping_strategy: CommentGroupingStrategy::PerConstantPerLocation,
-      root_path: nil
+      root_path: nil,
+      modularization_library: 'packwerk'
     )
       offenses_formatter ||= Check::DefaultFormatter.new
       repo_link = github.pr_json[:base][:repo][:html_url]
@@ -136,7 +138,12 @@ def check(
         line_number = reference_offense.location&.line
         referencing_file = reference_offense.reference.relative_path
 
-        message = offenses_formatter.format_offenses(unique_packwerk_reference_offenses, repo_link, org_name)
+        message = offenses_formatter.format_offenses(
+          unique_packwerk_reference_offenses,
+          repo_link,
+          org_name,
+          modularization_library: modularization_library
+        )
         markdown(message, file: git_filesystem.convert_to_filesystem(referencing_file), line: line_number)
       end
 

lib/danger-packwerk/update/default_formatter.rb

@@ -15,8 +15,8 @@ def initialize(custom_help_message: nil)
         @custom_help_message = custom_help_message
       end
 
-      sig { override.params(offenses: T::Array[BasicReferenceOffense], repo_link: String, org_name: String).returns(String) }
-      def format_offenses(offenses, repo_link, org_name)
+      sig { override.params(offenses: T::Array[BasicReferenceOffense], repo_link: String, org_name: String, modularization_library: String).returns(String) }
+      def format_offenses(offenses, repo_link, org_name, modularization_library: 'packwerk')
         violation = T.must(offenses.first)
         referencing_file_pack = ParsePackwerk.package_from_path(violation.file)
         # We remove leading double colons as they feel like an implementation detail of packwerk.
@@ -28,7 +28,11 @@ def format_offenses(offenses, repo_link, org_name)
 
         package_referring_to_constant_owner = Private::OwnershipInformation.for_package(referencing_file_pack, org_name)
 
-        disclaimer = 'We noticed you ran `bin/packwerk update-todo`. Check out [the docs](https://github.com/Shopify/packwerk/blob/main/RESOLVING_VIOLATIONS.md) to see other ways to resolve violations.'
+        if modularization_library == 'packwerk'
+          disclaimer = 'We noticed you ran `bin/packwerk update-todo`. Check out [the docs](https://github.com/Shopify/packwerk/blob/main/RESOLVING_VIOLATIONS.md) to see other ways to resolve violations.'
+        elsif modularization_library == 'pks'
+          disclaimer = 'We noticed you ran `bin/pks update`. Check out [the docs](https://github.com/Shopify/packwerk/blob/main/RESOLVING_VIOLATIONS.md) to see other ways to resolve violations.'
+        end
         pluralized_violation = offenses.count > 1 ? 'these violations' : 'this violation'
         request_to_add_context = "- Could you add some context as a reply here about why we needed to add #{pluralized_violation}?\n"
 

lib/danger-packwerk/update/offenses_formatter.rb

@@ -14,10 +14,11 @@ module OffensesFormatter
         abstract.params(
           offenses: T::Array[BasicReferenceOffense],
           repo_link: String,
-          org_name: String
+          org_name: String,
+          modularization_library: String
         ).returns(String)
       end
-      def format_offenses(offenses, repo_link, org_name); end
+      def format_offenses(offenses, repo_link, org_name, modularization_library: 'packwerk'); end
     end
   end
 end

spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb

@@ -29,10 +29,12 @@ module DangerPackwerk
       subject do
         danger_package_todo_yml_changes.check(
           before_comment: before_comment,
-          root_path: root_path
+          root_path: root_path,
+          modularization_library: modularization_library
         )
       end
 
+      let(:modularization_library) { 'packwerk' }
       let(:some_pack_package_todo_before) { nil }
       let(:diff_double) { sorbet_double(Git::Diff::DiffFile) }
 
@@ -139,14 +141,42 @@ module DangerPackwerk
               EXPECTED
             ).and_nothing_else
           end
+
+          context 'a modularization_library is passed in' do
+            let!(:modularization_library) { 'pks' }
+
+            it 'displays a markdown with a reasonable message' do
+              subject
+
+              expect('packs/some_pack/package_todo.yml').to contain_inline_markdown(
+                <<~EXPECTED
+                  ---
+                  packs/some_other_pack:
+                    "OtherPackClass":
+                      violations:
+                      - privacy
+                      - dependency
+                      files:
+                      - packs/some_pack/some_class.rb
+                  ==================== DANGER_START
+                  Hi again! It looks like `OtherPackClass` is private API of `packs/some_other_pack`, which is also not in `packs/some_pack`'s list of dependencies.
+                  We noticed you ran `bin/pks update`. Check out [the docs](https://github.com/Shopify/packwerk/blob/main/RESOLVING_VIOLATIONS.md) to see other ways to resolve violations.
+
+                  - Could you add some context as a reply here about why we needed to add these violations?
+
+                  ==================== DANGER_END
+                EXPECTED
+              ).and_nothing_else
+            end
+          end
         end
 
         context 'a offenses formatter is passed in' do
           let(:offenses_formatter) do
             Class.new do
               include Update::OffensesFormatter
 
-              def format_offenses(added_violations, repo_link, org_name)
+              def format_offenses(added_violations, repo_link, org_name, modularization_library: 'packwerk')
                 <<~MESSAGE
                   There are #{added_violations.count} new violations,
                   with class_names #{added_violations.map(&:class_name).uniq.sort},
@@ -1160,7 +1190,7 @@ def format_offenses(added_violations, repo_link, org_name)
             Class.new do
               include Update::OffensesFormatter
 
-              def format_offenses(added_violations, repo_link, org_name)
+              def format_offenses(added_violations, repo_link, org_name, modularization_library: 'packwerk')
                 <<~MESSAGE
                   There are #{added_violations.count} new violations,
                   with class_names #{added_violations.map(&:class_name).uniq.sort},

spec/danger_packwerk/danger_packwerk_spec.rb

@@ -30,7 +30,7 @@ module DangerPackwerk
           Class.new do
             include Check::OffensesFormatter
 
-            def format_offenses(offenses, repo_link, org_name)
+            def format_offenses(offenses, repo_link, org_name, modularization_library:)
               offenses.map(&:message).join("\n\n")
             end
           end
@@ -775,7 +775,8 @@ def format_offenses(offenses, repo_link, org_name)
             grouping_strategy: DangerPackwerk::PerConstantPerPackGrouping,
             offenses_formatter: Check::DefaultFormatter.new(
               custom_help_message: 'Need help? Join us in #ruby-modularity or see go/packs.'
-            )
+            ),
+            modularization_library: modularization_library
           )
         end
 
@@ -816,6 +817,7 @@ def format_offenses(offenses, repo_link, org_name)
           YML
         end
 
+        let(:modularization_library) { 'packwerk' }
         let(:danger_packwerk) { dangerfile.packwerk }
         let(:generic_privacy_violation) do
           sorbet_double(
@@ -889,6 +891,46 @@ def format_offenses(offenses, repo_link, org_name)
             expect(actual_markdown.type).to eq :markdown
           end
 
+          context 'modularization_library is pks' do
+            let(:modularization_library) { 'pks' }
+
+            it 'uses the pks update command' do
+              subject
+              expect(dangerfile.status_report[:warnings]).to be_empty
+              expect(dangerfile.status_report[:errors]).to be_empty
+              actual_markdowns = dangerfile.status_report[:markdowns]
+              expect(actual_markdowns.count).to eq 1
+              actual_markdown = actual_markdowns.first
+              expected = <<~EXPECTED
+                **Packwerk Violation**
+                - Type: Privacy :lock:
+                - Constant: [<ins>`PrivateConstant`</ins>](https://github.com/MyOrg/my_repo/blob/main/packs/gusto_slack/app/services/private_constant.rb)
+                - Owning pack: packs/gusto_slack
+                  - Owned by [<ins>@MyOrg/product-infrastructure</ins>](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) (Slack: [<ins>#prod-infra</ins>](https://slack.com/app_redirect?channel=prod-infra))
+
+                <details><summary>Quick suggestions :bulb:</summary>
+
+                Before you run `bin/pks update`, check out these quick suggestions:
+                - Does the code you are writing live in the right pack?
+                  - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb`
+                - Does PrivateConstant live in the right pack?
+                  - If not, try `bin/packs move packs/destination_pack packs/gusto_slack/app/services/private_constant.rb`
+                - Does API in packs/gusto_slack/public support this use case?
+                  - If not, can we work with [<ins>@MyOrg/product-infrastructure</ins>](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) to create and use a public API?
+                  - If `PrivateConstant` should already be public, try `bin/packs make_public packs/gusto_slack/app/services/private_constant.rb`.
+
+                </details>
+
+                _Need help? Join us in #ruby-modularity or see go/packs._
+              EXPECTED
+
+              expect(actual_markdown.message).to eq expected
+              expect(actual_markdown.line).to eq 12
+              expect(actual_markdown.file).to eq 'packs/referencing_package/some_file.rb'
+              expect(actual_markdown.type).to eq :markdown
+            end
+          end
+
           context 'there is no owning team' do
             before do
               allow(CodeOwnership).to receive(:for_package).and_return(nil)