Feedback and Contributions Welcome for Rust Implementation of Packwerk: Packs

exactly, those three are inflections :sweat_smile: Personally speaking, I wouldn’t like to remove them.

Apart from those there are two other cases: ::B2b::FindLeadForUserService , ::E18n

is it possible that it’s because of the numbers within their names? :zany_face:

Message originally sent by slack user U717FXJ8HWK

I also ran into the problem with a module that has a number in the name, somehow packs thinks the module belongs to a pack even though it’s in the root pack. :thinking_face: Not sure what’s going on here. packwerk doesn’t exhibit this problem, no matter which application generates the cache.

Message originally sent by slack user U717FXJ8HWK

It’s very fast indeed, here are my results (single runs, probably very noisy):

Uncached: Ruby 13.613s, Rust 1.607s.
Cached: Ruby 9.441s, Rust 0.930s.

<@U717FXJ8HWK> What’s the name of the constant and the filename defining the constant? Curious why packs has determine the constant is defined in a different place than packwerk.

Message originally sent by slack user U717FXJ8HWK

Sent you a PM

For anyone curious, Oleg’s problem was that he had a constant:

MyModule::MySubmodule5a::MyConstant

That file was defined in packs/pack1/my_module/my_submodule_5a/my_constant.rb
packwerk thinks that file defines the constant defined above. packs thought it defined …::MySubmodule5A::…
So packs, (like packwerk), figured the constant must be defined in a parent namespace, in this case .../my_module.rb, which was defined in a different pack than …/my_constant.rb

A solution is to convert files to class names using the same algorithm that packwerk uses (letters following numerics should be lowercase, not uppercase)

Message originally sent by slack user U70TIGAX94P

Packwerk aimed to replicate zeitwerk conventions on this. I think that‘s the right benchmark to look at, in case there is a bug in packwerk.

Yeah, for the most part I’m just hoping to replicate the way packwerk inflects constant and expects file names to define constants based on those inflections. I did see some idiosyncrasies though (where packwerk thinks a file must define constant ABC , but it defines Abc, so packwerk doesn’t detect a violation, but zeitwerk still loads the file fine).

I think it’s possible to totally side-step this problem by just parsing definitions directly (rather than inferring definitions from file names). I think this will actually result in less surprising behavior, faster performance, the ability to use the tool in non-zeitwerk enabled gems, and the ability to understand that some constants come from gems (by parsing RBIs using the same logic).

(some of those points need some unpacking, but it’d be a big conceptual change requiring a lot more buy in)

Message originally sent by slack user U70TIGAX94P

The reason packwerk doesn’t parse files for definitions is editor integration and optimization for single file scanning.

Packwerk actually already parses files for definitions, but then throws them away! It does that so it can collect “local definitions,” which are ignored as an early optimization (to save time during checking).

https://github.com/Shopify/packwerk/blob/main/lib/packwerk/parsed_constant_definitions.rb

You’re right though that when scanning a single file, we still need to collect all definitions, which is not feasible in the ruby implementation, so we need to build the constant resolver to quickly get all definitions.

With the rust implementation, this is not really much of a problem anymore, especially if we cache definitions as well as references.

<@U717FXJ8HWK> and @santib Would you be willing to try again with the newest version of packs? https://github.com/alexevanczuk/packs

It’s designed to parse config/initializers/inflections.rb , and it shouldn’t have the same issue with digits!

This version actually appears to work 100% for Gusto’s monolith, but you may still have some issues.

Message originally sent by slack user U717FXJ8HWK

I’ve already tried it in the morning :sparkles: The numbers issue is gone, the initializer issue is still there though. That’s the one with a file not being read at all

Is that the only issue left for you?

Message originally sent by slack user U717FXJ8HWK

I think so, yes

Message originally sent by slack user U717FXJ8HWK

No git diff except for one single violation that is removed