We can wait until 777 goes in, but any smaller PRs should just roll with it.
I’ve found (after doing it wrong a couple times) that if you just do the simple 
update merge:
    #in local git repo branch METRON-XXX, with additional git remote ‘apache’
    git fetch apache
    git merge apache/master
    #resolve merge conflicts
    git commit
    git push origin METRON-XXX

then github very nicely doesn’t make the reviewers for the PR review all the 
additional files that got changed by the merge (unless they want to).  So it’s 
low cost.

So to be clear, we should wait for 777, then let Justin do the commit, and all 
open PRs immediately do an update merge – which in the vast majority of cases 
will be fully automatic in the merge, and unnecessary to review in the PR.
--Matt

From: Otto Fowler <ottobackwa...@gmail.com>
Date: Friday, August 11, 2017 at 11:23 AM
To: Matt Foley <mfo...@hortonworks.com>, "dev@metron.apache.org" 
<dev@metron.apache.org>
Subject: Re: [DISCUSS] Code Reformatting next steps

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<mailto: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<mailto: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