metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Matt Foley <mfo...@hortonworks.com>
Subject Re: [DISCUSS] Code Reformatting next steps
Date Mon, 14 Aug 2017 21:48:39 GMT
Sure, my suggestion was intended to minimize or remove project management overhead.  If you’re
willing to shepherd through the list of 40-ish modules, go for it. The same work gets done
at the end.

Suggest extracting a checker script from your fixer script, add it to the build utils, and
run it at license check or similar time – after we think the work is done, to keep old style
from sneaking back in.
--Matt

On 8/14/17, 5:15 AM, "Justin Leet" <justinjleet@gmail.com> wrote:

    Unfortunately, I did not getting around to splitting out the changes over
    the weekend.
    
    Matt, it looks like your reply was pretty close to my email, so I'm not
    sure you saw it.  Would you have objections to just doing the license fix
    and reformat as paired tickets, module-by-module?  I think that'll just be
    easier all around to coordinate, and in retrospect I think it'll make my
    life easier too.  At that point, I'll just kill the existing PR and put out
    new ones when I have some free time.
    
    On Fri, Aug 11, 2017 at 2:42 PM, Matt Foley <mfoley@hortonworks.com> wrote:
    
    > 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 <ottobackwards@gmail.com>
    > Date: Friday, August 11, 2017 at 11:23 AM
    > To: Matt Foley <mfoley@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 (mfoley@hortonworks.com<mailto:
    > mfoley@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" <justinjleet@gmail.com<mailto:
    > justinjleet@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
    >
    >
    

Mime
View raw message