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