Default namespace support #27
Conversation
| @@ -119,6 +119,15 @@ def config | |||
|
|
|||
| private | |||
|
|
|||
| def coerce_load_paths(load_paths) | |||
| load_paths = Hash[load_paths.map { |p| [p, "Object"] }] unless load_paths.respond_to?(:transform_keys) | |||
There was a problem hiding this comment.
At some point I'd like to introduce sorbet to this codebase to make these things a little more obvious, but this is fine for now
There was a problem hiding this comment.
Ha, yeah, I was thinking the same thing but that's a PR for another day 😄
| load_paths = Hash[load_paths.map { |p| [p, "Object"] }] unless load_paths.respond_to?(:transform_keys) | ||
|
|
||
| load_paths.transform_keys! { |p| p.end_with?("/") ? p : p + "/" } | ||
| load_paths.transform_values! { |ns| ns.to_s.delete_prefix("::") } |
There was a problem hiding this comment.
Can you elaborate on why this is necessary?
There was a problem hiding this comment.
This is an attempt to ensure all default namespace strings are formatted the same and in the way as expected by the resolve method.
mclark
force-pushed
the
default-namespace-support
branch
from
5d1d8dc to
32f964a
Compare
|
Oops! Bad etiquette, I should not have force pushed after a review ☝️ All that was doing was fixing a style linting error, adding a comma to the last element of an array. |
|
Let's see if we can get a second review within a few days, but if not, I'm OK shipping this as is |
As described in Shopify/packwerk#186, Zeitwerk supports default modules for root directories. This pull request adds support for this capability to
ConstantResolver.I opted to keep this as simple as possible and just deal with Strings for the constant names. The resolver appears to inherently only be concerned with Strings anyway so it seemed to be consistent to me versus holding references to
Moduleinstances. If the provided load paths hash points to modules, as it does in the hash returned byZeitwerk::Loader#root_dirs, then we just callto_son them anyway.For detecting whether the load paths are a hash or not, I duck check to see if the argument responds to
:transform_keys. I'm not sure if this is ideal, maybe others have ideas?I've also tested this change in concert with the Packwerk changes on this branch against the GitHub monolith to ensure things behave as expected. The GitHub monolith is old and has a few root directories which have default namespaces other than
Objectso this is a decent test case. I'm pleased to report the expected violations were detected resulting much more accurate reports.cc @exterm @rafaelfranca from our discussions on Shopify/packwerk#186