[#221] Rename deprecated_references.yml to package_todo.yml #242

alexevanczuk · 2022-10-25T13:35:23Z

What are you trying to accomplish?

This addresses #221
More detail in the thread about the "why" for this change.
Also see the thread for the "when" on this change (i.e. when would we want to merge it, since it breaks public API).

What approach did you choose and why?

As agreed on in the thread, I chose package_todo.yml as the name for these new files.
I added a new bin/packwerk todo command. I also considered the subcommands update, update-todo, but as this command is run pretty often, I wanted it to be really easy to type, and I didn't think the addition of "update" added much clarity anyways.
I double-read the old and the new file. Initially, I wanted to release this in three steps (as listed in the discussion). However, there is too much output that refers to package_todo.yml that I thought it would be confusing to users to not see the warning but to see references to this new file they didn't know about. Therefore I thought it was better UX to do this in two steps: change and warn and then remove the old thing.

What should reviewers focus on?

Let me know if this sequencing makes sense.

Type of Change

Bugfix
New feature
Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

Breaking change (fix or feature that would cause existing functionality to change)

To transition to this version, it's just a matter of renaming deprecated_references.yml files in your project.

Dir['**/deprecated_references.yml'].each{|f| File.rename(f, f.gsub('deprecated_references', 'package_todo')) }

Checklist

I have updated the documentation accordingly.
I have added tests to cover my changes.
It is safe to rollback this change.

alexevanczuk · 2022-10-25T13:38:47Z

RESOLVING_VIOLATIONS.md


 ### Emergency Fixes
 ❔ Is it a revert or is there a lot of urgency because you are fixing a production bug impacting customers?

-➡️ Simply run `bin/packwerk update-deprecations`, and address the violation when the customer issue is resolved.


Before there were 19 characters to type, now there are 4!

alexevanczuk · 2022-10-25T13:39:24Z

USAGE.md

@@ -235,28 +235,28 @@ See: [RESOLVING_VIOLATIONS.md](RESOLVING_VIOLATIONS.md)

 For existing codebases, packages are likely to have existing boundary violations.

-If so, you will want to stop the bleeding and prevent more violations from occuring. The existing violations in the codebase can be recorded in a [deprecated references list](#Understanding_the_list_of_deprecated_references) by executing:
+If so, you will want to stop the bleeding and prevent more violations from occuring. The existing violations in the codebase can be recorded in a [todo list](#understanding-the-package-todo-file) by executing:


I love "todo list" rather than "deprecated references list"

alexevanczuk · 2022-10-25T13:40:09Z

lib/packwerk/offense_collection.rb

+      if Pathname.new(@root_path).join(package.name, "deprecated_references.yml").exist?
+        warning = <<~WARNING
+          DEPRECATION WARNING: `deprecated_references.yml` files have been renamed to
+          `package_todo.yml`. Please see https://github.com/Shopify/packwerk/releases


I think the release should contain information about upgrading/migrating.

We could also create a temporary command like bin/packwerk migrate_deprecated_references_to_package_todo or something that looks through all packages and does this for the user, but not sure that's necessary.

Is it possible to rename them automatically when we run check or update?

You'll want a require for Pathname at the top of this file since we're using it now.

Since we depend on Active Support, we should probably leverage it for deprecation warnings.

It is possible, but I'd prefer not to do this. I think this would be really surprising for users.

Done!

This is the same API we use in lib/packwerk/sanity_checker.rb and lib/packwerk/configuration.rb for warning about deprecated API, so I wanted to be consistent with that.

It is possible, but I'd prefer not to do this. I think this would be really surprising for users.

If you log a message when it happens explaining the change I don't feel like it would be confusing. We wouldn't have to code an extra command or point to release notes if it just worked. We could also do this in a separate PR.

This is the same API we use in lib/packwerk/sanity_checker.rb and lib/packwerk/configuration.rb for warning about deprecated API, so I wanted to be consistent with that.

Noted. We should be consistent 👍

Okay! I'm down to play around with it in a separate PR. We can feel out what makes most sense there.

I'd honestly be happy with just a 1 line command in the docs that folks could copy/paste and run. Bringing that logic into the code base introduces other complications that may conflict with existing bugs around deciding when to delete these files.

gmcgibbon · 2022-10-26T04:08:55Z

lib/packwerk/cli.rb

@@ -67,7 +69,8 @@ def execute_command(args)
          Subcommands:
            init - set up packwerk
            check - run all checks
-            update-deprecations - update deprecated references
+            todo - update package_todo.yml files


Personally, I think update-todo with a shorthand of update would make the most sense. From rubocop's perspective, it is regenerate-todo. Since todo by itself isn't a verb, I would suggest keeping the update wording in the command.

Ah yeah I see your point. I really loved the terseness of todo so thought maybe it was worth the tradeoff (of it not being a verb) since it's used so often, but update is only two more letters I suppose!! I'll change to update and update-todo.

Naming is hard! 😅 Normally I'd leave a conversation like this untouched as there's rarely value in muddying the waters, but this is the name of a subcommand and therefore pretty important so I thought I'd share my thoughts.

I also appreciate plain todo as it is brief and the intentions are clear. Grammatical consistency between subcommands is nice to have but not a hill I'd die on. update-todo feels a little unnecessarily wordy to me.

@gmcgibbon thanks for bringing up Rubocop's prior art! I also appreciate consistency and as Packwerk is very similar to Rubocop from the beginner's perspective, copying it is definitely a good thought.

Finally, another option that could work is dropping the todo command entirely and just having packwerk check --todo. Essentially updating the todo files is just a side effect or mode of a check anyway... 🤔

My preferences, in order would be:

packwerk check --todo

packwerk check --regenerate-todo

packwerk todo

packwerk regenerate-todo

packwerk update-todo

packwerk update feels too vague to me so I think I'd prefer to not support it at all honestly.

Thanks for sharing your thoughts Matt.

What if we make packwerk update-todo the long-form, and packwerk todo the shorthand? Would both of you be in support of that?

Personally todo doesn't make sense to me because it ins't a verb and may be misleading due to the mutative nature of the command. todo also feels wrong because the root of the command is "updating" the todo, so update makes sense to me as a partial completion of the full name and the verb of what you're actually doing.

I'm a fan of packwerk check --update-todo or --regenerate-todo though. Since there's more to type, we can settle on update for now and change it later if we find it necessary. If this is all about typing less characters we can always introduce packwerk u.

I just realized why I thought todo intuitively made sense!

It's because I use it fairly regularly as part of the workflow for Tapioca, another of Shopify's excellent tools! tapioca todo is used to generate any constants that can't be resolved in order to allow Sorbet to successfully type check the codebase. This creates a "burn down" list of missing constants that we have been resolving over time, much like package dependency and privacy violations are currently stored in our deprecated_references.yml file.

I still think todo makes the most sense but like I said, if you feel strongly in favour of update then I won't argue further.

Regardless, I just want to thank you both for caring so much about getting it right, it's good to work with folks who want the best for their projects.

Thanks @gmcgibbon and @mclark for both sharing more thoughts. Although I merged this, if @gmcgibbon agrees with being consistent with tapioca, I could put up a PR to switch over, given we haven't released anything yet (only merged this PR). Otherwise, we can leave as is.

gmcgibbon · 2022-10-26T04:12:01Z

lib/packwerk/offense_collection.rb

+      if Pathname.new(@root_path).join(package.name, "deprecated_references.yml").exist?
+        warning = <<~WARNING
+          DEPRECATION WARNING: `deprecated_references.yml` files have been renamed to
+          `package_todo.yml`. Please see https://github.com/Shopify/packwerk/releases


Is it possible to rename them automatically when we run check or update?

You'll want a require for Pathname at the top of this file since we're using it now.

Since we depend on Active Support, we should probably leverage it for deprecation warnings.

alexevanczuk · 2022-10-26T14:07:30Z

@gmcgibbon Responded to all your feedback and pushed up changes!

gmcgibbon · 2022-10-26T16:25:45Z

lib/packwerk/cli.rb

-            update-deprecations - update deprecated references
+            update-todo - update package_todo.yml files
+            update - Shorthand for update-todo
+            update-deprecations - update package_todo.yml files (deprecated, use todo instead)


Suggested change

update-deprecations - update package_todo.yml files (deprecated, use todo instead)

update-todo - update package_todo.yml files

Displaying deprecated commands isn't helpful. There is already documentation rot on this line, so let's just add this one line instead. We should be displaying a deprecation warning when we receive the deprecated command.

For the alias, it would be nice to have a set of aliases (like how bundle i/ins/inst/insta/instal/install works as bundle install) but I see no mention of this in the bundler docs. Just update and/or u would be fine IMO as undocumented shorthands.

While we're here, detect-stale-violations and generate_configs aren't mentioned. Do you know what we use them for?

Okay... sure! I'll remove the old deprecated commands and the shorthand.

I'm not totally sure what those are used but it might be worth auditing at some point 🤔 I think the former is used to basically find old violations that should be removed. Note sure about generate_configs without digging in further.

gmcgibbon · 2022-10-26T16:30:25Z

RESOLVING_VIOLATIONS.md


 ### Emergency Fixes
 ❔ Is it a revert or is there a lot of urgency because you are fixing a production bug impacting customers?

-➡️ Simply run `bin/packwerk update-deprecations`, and address the violation when the customer issue is resolved.
+➡️ Simply run `bin/packwerk update`, and address the violation when the customer issue is resolved.


Suggested change

➡️ Simply run `bin/packwerk update`, and address the violation when the customer issue is resolved.

➡️ Simply run `bin/packwerk update-todo`, and address the violation when the customer issue is resolved.

Please use the full command name in documentation.

Okay sure thing

mclark

Thank you both for putting up with my nitpicking opinions! Thank you Alex for shepherding this change, I think everyone is really happy with this improved naming! ✨

I added 👀 to a few places that had inconsistent command names, just to make sure we came back to them. I left a couple in there for fun as it seemed redundant to spam this PR with the emoji.

mclark · 2022-10-27T18:59:37Z

lib/packwerk/offense_collection.rb

+      if Pathname.new(@root_path).join(package.name, "deprecated_references.yml").exist?
+        warning = <<~WARNING
+          DEPRECATION WARNING: `deprecated_references.yml` files have been renamed to
+          `package_todo.yml`. Please see https://github.com/Shopify/packwerk/releases


I'd honestly be happy with just a 1 line command in the docs that folks could copy/paste and run. Bringing that logic into the code base introduces other complications that may conflict with existing bugs around deciding when to delete these files.

mclark · 2022-10-27T19:02:02Z

lib/packwerk/package_todo.rb

@@ -95,7 +95,7 @@ def dump
          #
          # You can regenerate this file using the following command:
          #
-          # bin/packwerk update-deprecations #{@package.name}
+          # bin/packwerk update #{@package.name}


mclark · 2022-10-27T19:02:36Z

test/integration/custom_executable_test.rb

@@ -73,58 +73,58 @@ class CustomExecutableTest < Minitest::Test
        assert_match(/No offenses detected/, captured_output)
      end

-      test "'packwerk update-deprecations' with no violations succeeds and updates no files" do
-        deprecated_reference_content = read_deprecated_references
+      test "'packwerk update' with no violations succeeds and updates no files" do


mclark · 2022-10-27T19:03:44Z

test/unit/parse_run_test.rb

      refute result.status
    end

-    test "#update_deprecations returns success when there are no offenses" do
+    test "#todo returns success when there are no offenses" do


gmcgibbon

LGTM after these two things are done.

gmcgibbon · 2022-10-27T21:10:35Z

lib/packwerk/cli.rb

+        output_result(parse_run(args).update_todo)
+      when "update-todo"
+        output_result(parse_run(args).update_todo)
+      when "todo"


Todo isn't a good shorthand because it isn't a verb. It should be update. You can also use "a", "b", "c" in the when clause to DRY this up.

Alrighty I changed it back. I felt like Matt C. had some convincing points here that I think are worth responding to, but would love to be able to move forward on this too.

lib/packwerk/cli.rb

alexevanczuk · 2022-10-27T21:18:48Z

Changed those two things @gmcgibbon !

gmcgibbon · 2022-10-27T21:27:37Z

lib/packwerk/cli.rb

@@ -57,7 +57,17 @@ def execute_command(args)
      when "detect-stale-violations"
        output_result(parse_run(args).detect_stale_violations)
      when "update-deprecations"
-        output_result(parse_run(args).update_deprecations)
+        warning = <<~WARNING


Suggested change

warning = <<~WARNING

warning = <<~WARNING.squish

gmcgibbon · 2022-10-27T21:27:57Z

lib/packwerk/cli.rb

-        output_result(parse_run(args).update_deprecations)
+        warning = <<~WARNING
+          DEPRECATION WARNING: `update-deprecations` is deprecated in favor of
+          `update-todo` or just `update`


Suggested change

`update-todo` or just `update`

`update-todo`.

gmcgibbon · 2022-10-27T21:28:38Z

lib/packwerk/offense_collection.rb

-      File.join(@root_path, package.name, "deprecated_references.yml")
+    def package_todo_file_for(package)
+      if Pathname.new(@root_path).join(package.name, "deprecated_references.yml").exist?
+        warning = <<~WARNING


Suggested change

warning = <<~WARNING

warning = <<~WARNING.squish

Should this be one line too?

[#221] Rename deprecated_references.yml to package_todo.yml

Since packwerk 2.3.0, deprecated_references.yml has been renamed to package_todo.yml. Shopify/packwerk#242 Therefore, packs-rails needs to be renamed as well to avoid confusion for new packwerk users.

alexevanczuk requested a review from a team as a code owner October 25, 2022 13:35

alexevanczuk force-pushed the ae-rename-deprecated-references branch from 67847fe to 84745b4 Compare October 25, 2022 13:35

alexevanczuk commented Oct 25, 2022

View reviewed changes

gmcgibbon reviewed Oct 26, 2022

View reviewed changes

alexevanczuk requested a review from gmcgibbon October 26, 2022 14:07

gmcgibbon reviewed Oct 26, 2022

View reviewed changes

mclark reviewed Oct 27, 2022

View reviewed changes

alexevanczuk force-pushed the ae-rename-deprecated-references branch from b33e202 to 122547e Compare October 27, 2022 20:34

gmcgibbon reviewed Oct 27, 2022

View reviewed changes

alexevanczuk requested a review from gmcgibbon October 27, 2022 21:18

gmcgibbon approved these changes Oct 27, 2022

View reviewed changes

Rename deprecated_references.yml to package_todo.yml

f4904e5

alexevanczuk force-pushed the ae-rename-deprecated-references branch from 7dc7e07 to f4904e5 Compare October 28, 2022 12:30

alexevanczuk merged commit 71da116 into main Oct 28, 2022

alexevanczuk deleted the ae-rename-deprecated-references branch October 28, 2022 12:35

gmcgibbon pushed a commit that referenced this pull request Feb 21, 2023

Merge pull request #242 from Shopify/ae-rename-deprecated-references

e653835

[#221] Rename deprecated_references.yml to package_todo.yml

euglena1215 mentioned this pull request Dec 3, 2023

Rename deprecated_references.yml to package_todo.yml rubyatscale/packs-rails#74

Merged

	update-deprecations - update package_todo.yml files (deprecated, use todo instead)
	update-todo - update package_todo.yml files

	➡️ Simply run `bin/packwerk update`, and address the violation when the customer issue is resolved.
	➡️ Simply run `bin/packwerk update-todo`, and address the violation when the customer issue is resolved.

[#221] Rename deprecated_references.yml to package_todo.yml #242

[#221] Rename deprecated_references.yml to package_todo.yml #242

Conversation

alexevanczuk commented Oct 25, 2022 • edited Loading

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

Type of Change

Additional Release Notes

Checklist

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

gmcgibbon Oct 26, 2022 • 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

gmcgibbon Oct 27, 2022 • 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

alexevanczuk commented Oct 26, 2022

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

mclark 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

gmcgibbon left a comment

Choose a reason for hiding this comment

gmcgibbon Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexevanczuk commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

alexevanczuk commented Oct 25, 2022 •

edited

Loading

gmcgibbon Oct 26, 2022 •

edited

Loading

gmcgibbon Oct 27, 2022 •

edited

Loading

mclark left a comment •

edited

Loading

gmcgibbon Oct 27, 2022 •

edited

Loading

gmcgibbon Oct 27, 2022 •

edited

Loading