+1 for import order On Wed, Aug 24, 2016 at 9:01 AM, Amit Sela <[email protected]> wrote:
> +1 on import order as well. > Kenneth has a good point about history if we reformat. > > On Wed, Aug 24, 2016, 18:59 Kenneth Knowles <[email protected]> > wrote: > > > +1 to import order > > > > I don't care about actually enforcing formatting, but would add it to IDE > > tips and just make it an "OK topic for code review". Enforcing it would > > result in obscuring a lot of history for who to talk to about pieces of > > code. > > > > And by the way there is a recent build of the IntelliJ plugin for > > https://github.com/google/google-java-format, available through the > usual > > plugin search functionality. I use it and it is very nice. > > > > On Tue, Aug 23, 2016 at 11:26 PM, Aljoscha Krettek <[email protected]> > > wrote: > > > > > +1 on the import order > > > > > > +1 on also starting a discussion about enforced formatting > > > > > > On Wed, 24 Aug 2016 at 06:43 Jean-Baptiste Onofré <[email protected]> > > wrote: > > > > > > > Agreed. > > > > > > > > It makes sense for the import order. > > > > > > > > Regards > > > > JB > > > > > > > > On 08/24/2016 02:32 AM, Ben Chambers wrote: > > > > > I think introducing formatting should be a separate discussion. > > > > > > > > > > Regarding the import order: this PR demonstrates the change > > > > > https://github.com/apache/incubator-beam/pull/869 > > > > > > > > > > I would need to update the second part (applying optimize imports) > > > prior > > > > to > > > > > actually merging. > > > > > > > > > > On Tue, Aug 23, 2016 at 5:08 PM Eugene Kirpichov > > > > > <[email protected]> wrote: > > > > > > > > > >> Two cents: While we're at it, we could consider enforcing > formatting > > > as > > > > >> well (https://github.com/google/google-java-format). That's a > > bigger > > > > >> change > > > > >> though, and I don't think it has checkstyle integration or > anything > > > like > > > > >> that. > > > > >> > > > > >> On Tue, Aug 23, 2016 at 4:54 PM Dan Halperin > > > > <[email protected]> > > > > >> wrote: > > > > >> > > > > >>> yeah I think that we would be SO MUCH better off if we worked > with > > an > > > > >>> out-of-the-box IDE. We don't even distribute an IntelliJ/Eclipse > > > config > > > > >>> file right now, and I'd like to not have to. > > > > >>> > > > > >>> But, ugh, it will mess up ongoing PRs. I guess committers could > fix > > > > them > > > > >> in > > > > >>> merge, or we could just make proposers rebase. (Since committers > > are > > > > most > > > > >>> proposers, probably little harm in the latter). > > > > >>> > > > > >>> On Tue, Aug 23, 2016 at 4:11 PM, Jesse Anderson < > > > [email protected] > > > > > > > > > >>> wrote: > > > > >>> > > > > >>>> Please. That's the one that always trips me up. > > > > >>>> > > > > >>>> On Tue, Aug 23, 2016, 4:10 PM Ben Chambers < > [email protected]> > > > > >> wrote: > > > > >>>> > > > > >>>>> When Beam was contributed it inherited an import order [1] that > > was > > > > >>>> pretty > > > > >>>>> arbitrary. We've added org.apache.beam [2], but continue to use > > > this > > > > >>>>> ordering. > > > > >>>>> > > > > >>>>> Both Eclipse and IntelliJ default to grouping imports into > > > alphabetic > > > > >>>>> order. I think it would simplify development if we switched our > > > > >>>> checkstyle > > > > >>>>> ordering to agree with these IDEs. This also removes special > > > > >> treatment > > > > >>>> for > > > > >>>>> specific packages. > > > > >>>>> > > > > >>>>> If people agree, I'll send out a PR that changes the checkstyle > > > > >>>>> configuration and runs IntelliJ's sort-imports on the existing > > > files. > > > > >>>>> > > > > >>>>> -- Ben > > > > >>>>> > > > > >>>>> [1] > > > > >>>>> org.apache.beam,com.google,android,com,io,Jama,junit,net, > > > > >>>> org,sun,java,javax > > > > >>>>> [2] com.google,android,com,io,Jama,junit,net,org,sun,java, > javax > > > > >>>>> > > > > >>>> > > > > >>> > > > > >> > > > > > > > > > > > > > -- > > > > Jean-Baptiste Onofré > > > > [email protected] > > > > http://blog.nanthrax.net > > > > Talend - http://www.talend.com > > > > > > > > > >
