Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposed changes to use more of Moo, second attempt #17

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

dakkar
Copy link

@dakkar dakkar commented Dec 26, 2017

(This PR contains the same changes as #15, but organised in a way that should make it much easier to review)

In reference to #14 :

  • type constraints everywhere: among other things, this makes clearer what each attribute can accept
  • defaults for all attributes where it makes sense
  • BUILDARGS/new instead of common/build, this makes classes easier to use and extend
  • op now canonically accepts a coderef (previously it did, but probably by mistake: it's a useful feature, let's support it properly)

Maybe needed before merging:

  • documentation for all the type constraints
  • better names for them

Possible future work:

  • review the various enums
  • review the various union types, and check if they really make sense
  • reduce the number of rw attributes

the munging of values passed into ->new should be done in BUILDARGS,
which is invoked automatically by Moo.
I kept the `sub build` in case someone was using them, but they just
delegate to `->new` now.

I had to add some filters into ::Command, because `->build` accepted
blessed objects (and took them apart, and rebuild identical objects),
but `->new` only accepts un-blessed hashrefs (or hashes, but that's
not the issue here)
attributes that should only hold instance of App::Spec are now
constrained, and will throw exceptions when set to something different
I've left some attributes without `isa`, because they require more
complex type constraints; I'll get back to those later
->callbacks is a hashref of arrayrefs
BUILDARGS must now make sure it doesn't return `undef` values, because
they won't pass the constraints.

I've had to change `has_subcommands` to handle the case of an empty
hashref (it worked more-or-less by accident, before)
this constrains values within the set we can handle (e.g. markup can
only be `'pod'` or `'swim'`)
13.argv.t used an arrayref instead of a hashref for `->callbacks`,
fixed it
These are awkward, as they have to capture all the various ways these
attributes' values can be shaped. I have covered all the types I could
discern from the tests and code, I hope I caught them all
with type constraints, there are no longer useful
coderefs always worked, there's no reason to only accept method names
that block of code takes user-provided plugin names, and converts them
into a form that's valid for the class; that's exactly what BUILDARGS
is for
`BUILD` is invoked automatically from `->new`, to finish initialising
the object, so those calls should be there: every object requires them!

the trick with declaring the sub and them applying a modifier to it is
needed because we're in a role: if the class that consumes the role
declares its own `BUILD`, the one provided by the role won't be
installed. This way, instead, the empty one won't be installed, but
the modifier will still be applied, and we always get the correct
behaviour
@dakkar
Copy link
Author

dakkar commented Dec 26, 2017

I suggest looking at one commit at a time, in order: I've tried to keep them self-contained (tests always pass, for example) and focused (one file, or one class of type constraints per commit, for example).

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.

1 participant