Skip to content

Conversation

@shageman
Copy link
Contributor

What are you trying to accomplish?

While working on #218 I noticed that there is a bit of cruft in the methods that are working together to determine the load paths to be included by packwerk. They pass around hashes of loaders, when all that is needed is pathname (sometimes as Pathname, sometimes as String).

What approach did you choose and why?

I switched the params from using hashes to using arrays to clarify the intent of what is going on.

What should reviewers focus on?

Nothing specific

Type of Change

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

Checklist

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

@shageman shageman requested a review from a team as a code owner August 11, 2022 15:10
Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

Love the simplification. Thank you

It seems the email you used to commit isn't attached to your GitHub account, so our CLI bot can't find if you signed it or not. Do you mind to fix that?

@shageman
Copy link
Contributor Author

shageman commented Aug 11, 2022

I would love to. However, I had to change the email address associated with my account. I suspect Shopify needs to change the primary key behind the CLA form from github_username to github_username+email, so I can fill the CLA out again (which I don't mind doing)... or something like that.

As I said on the referenced PR, I think I am blocked from filling out the form again.

@rafaelfranca
Copy link
Member

I think you GitHub account can have many associated emails. So even if you changed, you can still associated the email you used to commit to the GitHub account. I can see if I can find someone to change the CLA system. But another alternative would be to change the commit to use the email now associated with your account.

@shageman
Copy link
Contributor Author

Ah, ok. I will take a look.

@shageman
Copy link
Contributor Author

@rafaelfranca thanks for the tip! That worked!

@rafaelfranca rafaelfranca merged commit 87ea54b into Shopify:main Aug 11, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems August 11, 2022 16:30 Inactive
@mclark
Copy link
Contributor

mclark commented Aug 11, 2022

I saw this one too late, but the idea behind those hashes is to preserve Packwerk's knowledge of the "default namespace" for each root directory. GitHub relies on this feature for a number of odd older load dirs.

Can this be reverted?

@rafaelfranca
Copy link
Member

rafaelfranca commented Aug 11, 2022

Of course. But I don't see that information being used by packwerk itself, is it? How are you using this from inside Packwerk?

@mclark
Copy link
Contributor

mclark commented Aug 11, 2022

I'm about to board a plane at the moment but the original change is in #190 and has some context.

I'll investigate the full story later when I am back at a computer as it's been a while since I worked on that one. I suspect I probably messed up one of the tests in that PR.

@mclark
Copy link
Contributor

mclark commented Aug 12, 2022

So just to close the loop on this one, https://github.com/Shopify/packwerk/pull/190/files#diff-8e43c83d1320f09335dd0d66cb5cc51646ccd11cabf6a3a696b1b4411b415ffdR41 is the key change. Basically, constant resolver now supports a hash of load paths to default namespaces and Packwerk leverages that to support them for monoliths that use this behaviour.

I will see about adding a test to ensure this functionality is not lost again, it's disappointing nothing failed!

@shopify-shipit shopify-shipit bot temporarily deployed to rubygems November 15, 2022 00:55 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