Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix package_todo update comment #321

Merged
merged 1 commit into from
Mar 7, 2023
Merged

Fix package_todo update comment #321

merged 1 commit into from
Mar 7, 2023

Conversation

gmcgibbon
Copy link
Member

What are you trying to accomplish?

Fix update command in package TODOs.

What approach did you choose and why?

Remove the arguments from update. The update-todo subcommand with arguments has been deprecated and is no longer supported.

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)

Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.

If no additional notes are necessary, delete this section or leave it unchanged.

Checklist

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

@gmcgibbon gmcgibbon requested a review from a team as a code owner March 6, 2023 21:08
package_todo.dump

assert_equal(<<~YAML, file.readlines.first(7).join)
# This file contains a list of dependencies that are not part of the long term plan for buyers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand where this buyers string comes from.
I understand it's interpolated from @package.name here:

# This file contains a list of dependencies that are not part of the long term plan for #{@package.name}.

I don't understand where it's coming from though. Should the string itself be part of this test's setup?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is out of scope for this PR but I think we can make that message a bit less confusing by changing it to "the 'buyers' package".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put up a PR to follow up on that thought #323

Copy link
Contributor

@exterm exterm Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the original topic of this thread (sorry for the derailment), it seems that the string is from the destination_package as implemented at the bottom of the file and used at the top in the setup block. It's not very obvious because of the size of the test file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is from the destination package (see the method in the test case), which would be the one with the violations. It is just confusing in the test because we use tempfiles and mock the setup a bit.

The update-todo subcommand with arguments has been deprecated and is no
longer supported.
@gmcgibbon gmcgibbon force-pushed the fix_invalid_cmd_comment branch from 128d5fc to b7d9132 Compare March 7, 2023 19:58
@gmcgibbon gmcgibbon merged commit 34eb9f5 into main Mar 7, 2023
@gmcgibbon gmcgibbon deleted the fix_invalid_cmd_comment branch March 7, 2023 20:00
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems March 9, 2023 00:42 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants