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

Filter custom paths according to include #194

Merged
merged 1 commit into from
Jun 13, 2022
Merged

Filter custom paths according to include #194

merged 1 commit into from
Jun 13, 2022

Conversation

thomasmarshall
Copy link
Contributor

What are you trying to accomplish?

Custom paths are already filtered by the exclude option, but this commit ensures that the include option is respected too. This is particularly useful for the packwerk-vscode extension, which attempts to run packwerk check $filename on save.

For example:

include:
  - a/**/*.rb
  - b/**/*.rb

I would expect packwerk check c/some/file.rb to show the "No files found or given" message, as it would if exclude was set. This commit ensures that happens.

What approach did you choose and why?

I chose to always filter custom paths according to the include option, because that seemed to mirror the behaviour of the exclude option. However, there might be valid use-cases for packwerk check some/not/included/file.rb that I haven't considered. If that's a common use-case, maybe I should approach this differently? Maybe the packwerk-vscode extension should be making decisions about when to run packwerk check instead?

What should reviewers focus on?

Are there common/valid use-cases for packwerk check that I am making impossible with this change? I guess if someone is relying on the existing behaviour then this might be a breaking change?

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.

Custom paths are already filtered by the `exclude` option, but this
commit ensures that the `include` option is respected too. This is
particularly useful for the `packwerk-vscode` extension, which attempts
to run `packwerk check $filename` on save.

For example:

```
include:
  - a/**/*.rb
  - b/**/*.rb
```

I would expect `packwerk check c/some/file.rb` to show the "No files
found or given" message, as it would if `exclude` was set. This commit
ensures that happens.
@thomasmarshall thomasmarshall requested a review from a team as a code owner April 14, 2022 09:29
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

Rubocop works like this when applying cops to a list of specific files, so I think it is acceptable to assume Packwerk should too. 👍

@exterm
Copy link
Contributor

exterm commented May 6, 2022

It is a breaking change IMO, so at least we should bump the version accordingly.

@alexevanczuk @mclark Would this cause any problems at Gusto or Github?

@alexevanczuk
Copy link
Contributor

alexevanczuk commented May 7, 2022

I tried this on Gusto's codebase and bin/packwerk update-deprecations produced no changes to deprecated_references.yml files.

Note Gusto is also not yet on the version of sorbet that packwerk 2.1.1 requires, but to test this on our codebase, I just pulled this branch locally and made some small tweaks. Wanted to include what I did in case GitHub or Shopify needs to do the same.

In packwerk

In packwerk.gemspec

  1. Downgraded `sorbet-runtime to the version in our application, in our case:
spec.add_dependency("sorbet-runtime", '0.5.9889')

In our application

In Gemfile

  1. Moved packwerk to:
path('../packwerk') do
  gem 'packwerk'
end
  1. bundle update constant_resolver --conservative (we were on 0.1.5)
  2. bundle install

@mclark
Copy link
Contributor

mclark commented May 9, 2022

@thomasmarshall I needed to merge main into this branch to verify your fix against the GitHub monolith as we now require 2.2.0 and it appears this branch is based on a slightly older commit. I then tested with just an include path to verify your changes took (they did!) and then ran bin/packwerk update and bin/packwerk update-deprecations on our standard configuration. Everything appears to be working as expected 🎉

Thanks for the fix @thomasmarshall! 👍

@gmcgibbon gmcgibbon merged commit 76da5d8 into main Jun 13, 2022
@gmcgibbon gmcgibbon deleted the filter-paths branch June 13, 2022 05:15
@gmcgibbon
Copy link
Member

gmcgibbon commented Jun 13, 2022

This is still a breaking change, but I think it is a good change. The next release will have to be 3.0.0 to denote the breaking change.

Thanks!!

@shopify-shipit shopify-shipit bot temporarily deployed to rubygems July 12, 2022 00:06 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.

5 participants