Merged! Happy new year to you and everyone in the community too.
Jon On Mon, Dec 31, 2018 at 2:39 PM Gurkan Erdogdu <[email protected]> wrote: > Hello Jon > I have created new pull request. Could you please check, > https://github.com/apache/tomee/pull/334 > > Happy New Year! > > Regards. > Gurkan > > > On Mon, Dec 31, 2018 at 5:11 PM Jonathan Gallimore < > [email protected]> wrote: > > > Awesome, thanks. I'm ok with this change: > > > > - moduleLocations.removeIf(file -> !match((String) > modules, > > file)); > > - > > + for (final Iterator<File> i = > moduleLocations.iterator(); > > i.hasNext(); ) { > > + final File file = i.next(); > > + if (!match((String) modules, file)) { > > + i.remove(); > > + } > > + } > > > > I don't think anyone else has expressed an opinion one way or another, so > > I'd leave your for-loop as it is. > > > > Jon > > > > On Mon, Dec 31, 2018 at 2:07 PM Gurkan Erdogdu <[email protected]> > > wrote: > > > > > Hi Jon > > > I will update my patch to update wildcard import, loop and diamond <> > > > operator. > > > Regards > > > Gurkan > > > > > > On Mon, Dec 31, 2018 at 2:38 PM Jonathan Gallimore < > > > [email protected]> wrote: > > > > > > > Is there any particular reason for flipping the imports to wildcards? > > We > > > > tend to import the individual classes elsewhere in the codebase. My > > > > personal preference would be to leave those as individual imports. It > > > > doesn't make the import list that much longer, and I find its clearer > > > what > > > > is being imported and used. IDEs tend to fold those away unless > you're > > > > specifically looking at them too. > > > > > > > > I think like the other changes, and do think it looks neater and > easier > > > to > > > > read. > > > > > > > > I personally prefer > > > > > > > > map = new HashMap<>(); > > > > > > > > over > > > > > > > > map = new HashMap<Object, Object>(); > > > > > > > > but I don't think that's a big deal. > > > > > > > > I also note this change: > > > > > > > > - moduleLocations.removeIf(file -> !match((String) > > > modules, > > > > file)); > > > > - > > > > + for (final Iterator<File> i = > > > moduleLocations.iterator(); > > > > i.hasNext(); ) { > > > > + final File file = i.next(); > > > > + if (!match((String) modules, file)) { > > > > + i.remove(); > > > > + } > > > > + } > > > > > > > > Which is changing from a lambda to a loop. I'd probably have done it > > like > > > > this, but the effect is the same: > > > > > > > > final Iterator<File> iterator = > > > moduleLocations.iterator(); > > > > while (iterator.hasNext()) { > > > > final File file = iterator.next(); > > > > if (!match((String) modules, file)) { > > > > iterator.remove(); > > > > } > > > > } > > > > > > > > IntelliJ is definitely preferring the moduleLocations.removeIf, and > > will > > > > refactor both of those loops to it with one keystroke. I have the > what > > is > > > > probably an unpopular opinion which is that I actually prefer the > loop > > > over > > > > the lambda. The reasons are: > > > > > > > > 1. Backporting changes is much harder when you have to work around > > > language > > > > changes like this > > > > 2. I personally find lots of things crammed into one line harder to > > read, > > > > and in this particular case, there's a cast and a negation (so a > double > > > > negative) in the removeIf. > > > > > > > > Jon > > > > > > > > On Mon, Dec 31, 2018 at 10:21 AM Jonathan Gallimore < > > > > [email protected]> wrote: > > > > > > > > > Yep, I'll take a look. Thanks for the PR. > > > > > > > > > > Jon > > > > > > > > > > On Mon, Dec 31, 2018 at 9:10 AM Gurkan Erdogdu < > [email protected]> > > > > > wrote: > > > > > > > > > >> Hi > > > > >> I have created a PR (https://github.com/apache/tomee/pull/332) > for > > > > >> (TOMEE-2435) > > > > >> Simplify the Code in org.apache.openejb.OpenEjbContainer$Provider > > > > >> <https://issues.apache.org/jira/browse/TOMEE-2435> > > > > >> > > > > >> If anybody can check and merge, it is appreciated :) > > > > >> Regards. > > > > >> Gurkan > > > > >> > > > > > > > > > > > > > > >
