Regarding Matt's point, I can pretty easily split the manual vs automatic
changes.  I won't have time today, but I (probably) can Sunday.  Commits
should be relatively low velocity then, so it should mostly naturally occur.

Regarding PRs, I'm personally okay with best effort, and just correcting
any remainders during any reformats (or possibly pairing a license fix with
a reformat or whatever practical solution to the general idea we want).  I
expect there to be some drift as PRs come in, but I'm going with the idea
that perfect is the enemy of good.  At that point, we just do as little as
possible for 777, and just take care of it during or near that particular
reformat. Seems like it mostly comes with the potential overhead of pairing
some tickets together, which is annoying, but not too bad.

It might be better to just submit a paired license fix ticket + a reformat
by module, if we'd prefer that.  I could just kill current 1087, we pick a
module, and submit a license fix ticket/PR + a refactor ticket/PR.  At that
point, we should mostly stay out 777's way, and we don't stop commits for
such a widely spread PR.  It's pretty much the same amount of work, and is
probably cleaner all around.

Justin


On Fri, Aug 11, 2017 at 2:23 PM, Otto Fowler <ottobackwa...@gmail.com>
wrote:

> What do we do for PR’s?
>
> I don’t think I should run this on 777 for example while it is under
> review.  But, I don’t think running it as a last commit before landing is
> correct either.
>
> Will it be:
> PR as is ( if you haven’t reformatted, don’t )
> Follow on PR with only reformatting
> ??
>
>
>
> On August 11, 2017 at 14:17:35, Matt Foley (mfo...@hortonworks.com) wrote:
>
> Regarding METRON-1087, I’m in favor of freezing commits for a day, to let
> Justin re-run the script for METRON-1087 over all of current master, and
> commit it.
> Perhaps, for assurance, this commit should only include the fully-automated
> “vast majority”; the couple dozen files that needed manual fixes could be
> split out into a patch that gets manual review. I don’t have a problem
> saying +1 on what is evidently script-automated (L1:’s#/**#/*#’ and blank
> line after license block).
> All of us with open PRs will then have to update to master, but we do that
> periodically anyway. Github is really quite good about not causing extra
> review work for update merges.
> IMHO,
> --Matt
>
> On 8/11/17, 5:14 AM, "Justin Leet" <justinjl...@gmail.com> wrote:
>
> Now that METRON-746 <https://github.com/apache/metron/pull/577> is in, we
> have a consistent code formatting setup where (for the most part) it can be
> autoapplied.
>
> Barring one existing PR, the main thing is just picking a module, doing the
> format, and testing it out. Obviously, I'd like to avoid collisions with
> any major PRs out there (say METRON-777
> <https://github.com/apache/metron/pull/530>), so things like parsers are
> out.
> Does anybody have any thoughts on what should be grabbed first? Maybe
> metron-stellar since it's pretty easy to test and even though it gets PRs,
> they're typically fairly small and contained?
>
> The main hiccup before being able to do any blanket reformatting is this
> PR:
> METRON-1087: Adjust license headers to be comments instead of Javadoc
> <https://github.com/apache/metron/pull/685>
>
> Without it, all the license headers get reformatted in Javadoc style when
> autoformatted (this actually happened before, but since we didn't actually
> format our code much it was pretty benign). Once this is in, I'm fine doing
> any headers that sneak in via PRs, it's a pretty easy find/replace in the
> editor.
>
> Once we're confident this works fairly smoothly, it should be pretty easy
> to branch out and start hitting more modules in whatever priority order we
> want.
>
> Justin
>

Reply via email to