[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

The python black project.

On 2019-04-23 21:09:30 +0000 (+0000), Fox, Kevin M wrote:
> Yeah, pushing a patch following the patch going in for review is
> more what I was thinking. So, a patch that fixes the previous
> patch, not actually rewriting the patch.

Sure, that's essentially another of the options I mentioned in an
earlier message:

> > There is also the possibility that you simply run a periodic
> > autopep8 job against the codebase and have that job propose a
> > commit to the repository to apply code style changes if there
> > are any, but that's a bit of a messy hack as well.

The main reasons I consider it a "messy hack" is that it runs the
potential of increasing the number of commits in the repository
significantly depending on whether and how often you batch up the
autofixes, and makes it harder to `git blame` effectively since a
lot of changes are immediately followed by changes to those same
lines by a generic automation account. It also means we give up on
whatever benefits we might hope to gain from having consistency of
style in proposed changes, as folks can propose code in any old
format they like without regard for readability and it only gets
cleaned up and made readable after it's been reviewed and approved.
This strikes me as a slippery slope towards "it's okay if your code
has obvious errors, we can find and fix them after it's merged."

> Is it a problem for the commit to "come from" and be "signed off"
> by the automation itself?

No, we have automation propose other sorts of changes already. From
a legal standpoint nobody's ever complained that the machine hasn't
agreed to the ICLA or acknowledged the DCO. I think once we're
successful enough with artificial intelligence for our computers to
start filing patent infringement claims against us for the work
they've written, we can revisit that position.

If we wanted to have our existing auto-proposed changes use `git
commit --gpg-sign` I think that would be able to take advantage of
much of the same automated signing infrastructure we already
leverage for our Git tags and related release artifacts. That much
seems to me like it could be a good idea, if someone wants to do the
work to add it. It would in theory allow any observer to confirm
that the patches which claim to have originated within our
automation actually have.

> It surfaces the cleanup patches as just cleanup patches. And in
> the very end of the ps, we squash the pr on merge anyway? If the
> user wants, they can also just flatten the PS themselves and it
> would automatically merge in the automation provided patches.

Now it's starting to sound like you're suggesting auto-proposing
follow-up changes to each user-proposed change while it's still
under review. I think it's probably technically possible, though
likely to ~double our overall change volume insofar as review and
testing go (especially when folks inevitably ignore them and so they
get rebased and updated every time the parent change is updated).
And before you say "it's okay to skip running CI jobs on those" it's
really not, since we're not likely to blindly trust autopep8 not to
propose a subtly broken modification.
Jeremy Stanley
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 963 bytes
Desc: not available
URL: <http://lists.openstack.org/pipermail/openstack-discuss/attachments/20190423/d57394a6/attachment.sig>