search

alexevanczuk / packs

A pure Rust implementation of packwerk, a gradual modularization tool for Ruby
MIT License
69 stars 7 forks source link

something strange with load paths and active admin #146

Open exterm opened 7 months ago

exterm commented 7 months ago

trying to run pks on the app I'm working on right now, it complains about path collisions between packs/*/app/models/something.rb and packs/*/app/admin/something.rb. But app/admin isn't actually autoloaded. This is apparently the default pattern for ActiveAdmin.

So it seems that pks is somehow guessing the autoload paths incorrectly?

alexevanczuk commented 7 months ago

Hmm yeah, by default pks assumes that directory is always autoloaded: https://github.com/alexevanczuk/packs/blob/12fb39369f5b0dab0c4552f199f99caeb46a88ae/src/packs/pack.rs#L268

We haven't needed to configure this yet, so I don't think this is supported quite yet. What were ya thinking would make sense as an API to allow this to be configured?

exterm commented 7 months ago

The cleanest solution is to get the autoload paths from the app configuration...

How are you handling autoload paths from engines?

alexevanczuk commented 7 months ago

Not actually handling this at the moment because as you know it would require either loading Rails and talking to it somehow OR requiring client configuration to dump the paths as a separate process.

So far the issue hasn't come up because a lot of apps have really simple, convention-based paths and we could just implement that simple logic within pks.

exterm commented 7 months ago

yeah, I mean it's smart to do the guessing.

For the active admin problem specifically, you could add a config option to exclude certain folders from the autoload paths? It'd be a pretty specific piece of configuration.

Alternatively, if you accept a json or yaml file with load paths, or something like that, and document how to dump them...

But I guess this will no longer be necessary once the experimental parser is promoted to default?

mclark commented 7 months ago

In https://github.com/alexevanczuk/packs/pull/133 I added support for specifying autoload roots, with optional default namespaces. It's not documented yet, my bad. But it might at least start down the path to the support you need. I believe the roots support globbing as well.

autoload_roots:
  app/company_data: "::Company"

The only missing piece is that the default behaviour of assuming every path under app inside a package is an autoload root. We could probably add an option to disable that behaviour?

But I guess this will no longer be necessary once the experimental parser is promoted to default?

Yeah, that's our ultimate destination too, but we wanted to enable a gradual migration to it.

exterm commented 7 months ago

We could probably add an option to disable that behaviour?

Doesn't it make sense to implicitly disable it when autoload_roots are explicitly supplied?