I guess I probably ought not to have done this branch... I was just
itching to do something and I didn't have the presence of mind to back
off. Anyway, point by point:
> I'm kinda unhappy about this change, I'd have at least expected you
> to talk to me before just removing my work.
I figured you were away for the weekend now and probably unavailable,
it's easy to undo stuff, and I am hoping to seek forgiveness instead
of getting advance permission.
> I put the generated script in as a development convenience, because
> the bin/py example above is pretty hideous to remember and type. I
> intended the package to use the module's script as-is however,
> because it will have set up paths already.
The extra section in buildout.cfg wasn't needed for example, though
that's not much of an issue. Really I felt that this was the wrong way
to go about exposing commands. The conveniences we create for
ourselves as developers will inevitably flow downstream, and I would
like to avoid that in this case. The `python -m module` pattern
prevents PATH pollution and also keeps us aware that exposing
provisioning commands to users is not a well solved problem.
The Django side of MAAS has management commands. I think the
provisioning side should have something similar, a "command verb"
pattern, like django-admin or bzr for example.
Also, if we want to expose non-developer scripts, we ought to put them
in setup.py instead of buildout.cfg. I think this'll make them easier
for packagers to use.
> Also, the branch has removed a test which checks the stdout output,
I didn't realise that was what it was trying to test. I'll add a test
for this, but I'll use self.patch(sys, "stdout", ...) instead, if
that's okay?
> and the new name "writer" is inaccurate. If you want to split hairs,
> it's "write_config".
Every other package and module name, afaict, is a noun. Another
natural place for DHCPConfigWriter could be
provisioninserver.dhcp.scripts?
> I'd appreciate it if you'd put the script back, either
> buildout-generated or a shortcut using the bin/py method.
Okay.
> Why did you remove the build dependency from the check target?
> Regardless of my changes it seems a good idea.
Yeah, it is a good idea. This was actually the thing that got me
started on this branch: the massive rubber hammer to squash a single
missing dependency.
So, given that testing a full build is good, but that it's worthwhile
exercising the dependencies in Makefile too, I'll change it to do a
full build *after* running tests.
I guess I probably ought not to have done this branch... I was just
itching to do something and I didn't have the presence of mind to back
off. Anyway, point by point:
> I'm kinda unhappy about this change, I'd have at least expected you
> to talk to me before just removing my work.
I figured you were away for the weekend now and probably unavailable,
it's easy to undo stuff, and I am hoping to seek forgiveness instead
of getting advance permission.
> I put the generated script in as a development convenience, because
> the bin/py example above is pretty hideous to remember and type. I
> intended the package to use the module's script as-is however,
> because it will have set up paths already.
The extra section in buildout.cfg wasn't needed for example, though
that's not much of an issue. Really I felt that this was the wrong way
to go about exposing commands. The conveniences we create for
ourselves as developers will inevitably flow downstream, and I would
like to avoid that in this case. The `python -m module` pattern
prevents PATH pollution and also keeps us aware that exposing
provisioning commands to users is not a well solved problem.
The Django side of MAAS has management commands. I think the
provisioning side should have something similar, a "command verb"
pattern, like django-admin or bzr for example.
Also, if we want to expose non-developer scripts, we ought to put them
in setup.py instead of buildout.cfg. I think this'll make them easier
for packagers to use.
> Also, the branch has removed a test which checks the stdout output,
I didn't realise that was what it was trying to test. I'll add a test
for this, but I'll use self.patch(sys, "stdout", ...) instead, if
that's okay?
> and the new name "writer" is inaccurate. If you want to split hairs,
> it's "write_config".
Every other package and module name, afaict, is a noun. Another er.dhcp. scripts?
natural place for DHCPConfigWriter could be
provisioninserv
> I'd appreciate it if you'd put the script back, either
> buildout-generated or a shortcut using the bin/py method.
Okay.
> Why did you remove the build dependency from the check target?
> Regardless of my changes it seems a good idea.
Yeah, it is a good idea. This was actually the thing that got me
started on this branch: the massive rubber hammer to squash a single
missing dependency.
So, given that testing a full build is good, but that it's worthwhile
exercising the dependencies in Makefile too, I'll change it to do a
full build *after* running tests.