preaction / Statocles

Static website CMS
http://preaction.me/statocles
Other
84 stars 33 forks source link

Declare all dependencies of Statocles::Base to be Runtime_Requires + use META.json #468

Closed kentfredric closed 8 years ago

kentfredric commented 8 years ago

NB: This pull presently includes content from #467

Otherwise, everyone depending on Statocles::Base q[Test] will have a broken dependency series if:

This problem is currently hidden by the conflation of "test_requires" with "build_requires", so all CPAN Clients will install these dependencies that are currently marked "test only" despite the fact they're not running tests and opted --no-test.

This change becomes necessary with the introduction of META.json which CPAN clients can reliably use to indicate dependencies that are not strictly required, at which point, Test::Deep and friends being visible to the poor sucker who did only:

test_requires: { Statocles::Base => 0 }

Is likely to be a matter of dumb luck, not good design.

( This is one reason why I personally abstain from such magical modules as they add bugs in mixing unrelated concerns for the sake of convenience )

preaction commented 8 years ago

Right, depending on Statocles::Base in other dists is dangerous, for this among other reasons. To remove this specific confusion, the test bundle could be added to a t/lib module instead, combining with Statocles::Test. Then no way to get at the test requirements during runtime. Would that work instead?

kentfredric commented 8 years ago

If Statocles::Base removed ability to do use Statocles::Base 'Test', that would help I think.

As long as nobody else can use that functionality without embedding their own copy, then the dependency leak goes away. ( I'll attempt to scrub together something in a seperate pull )

kentfredric commented 8 years ago

Ok, there's still 2 dependencies that have to be promoted to "runtime" because they're used in Statocles::Test:

preaction commented 8 years ago

We could move those subs into the same t/lib module, or a different t/lib module. I no longer see the utility in providing those subs to other dists (though if you do, then this'll be fine as it is).

Combining an Import::Base module with an exporter isn't something I've ever tried, so if you get it to work, I'd love to add that to Import::Base's documentation.

kentfredric commented 8 years ago

Does this sort of thing look good enough?

https://github.com/kentfredric/Statocles/commit/880be32758f13f788b07a83c3db7b5cd3390905b

preaction commented 8 years ago

That'll work, but I was thinking just moving all of Statocles::Test into t/lib. Then it could be called Statocles::Test in there too (unless that's not a good idea).

kentfredric commented 8 years ago

The build_ stuff is actually kinda useful for writing Plugins.

I guess you could consider shipping a Statocles::Test independent of Statocles and then use a bundled copy of that library for testing Statocles itself.

preaction commented 8 years ago

Eh, better to leave it there and have nobody else use it than have someone copy/paste it. Looks good to me then.

kentfredric commented 8 years ago

Just gonna close this now because this direction is EOL.