PR review and merge

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

PR review and merge

brunobat
Reply | Threaded
Open this post in threaded view
|

Re: PR review and merge

Otávio Gonçalves de Santana
Reply | Threaded
Open this post in threaded view
|

Re: PR review and merge

Roberto Cortez
We didn’t.

If we are moving forward with the release, I would prefer to keep these in stand by until we are done. Right now, we are on a green build, and we can’t be sure if any of these PR’s would break it, so better to be safe :)

> On 24 Jan 2019, at 12:05, Otávio Gonçalves de Santana <[hidden email]> wrote:
>
> Hello everyone, please don't forget these PRs
>
> On Wed, Jan 23, 2019 at 8:01 AM Bruno Baptista <[hidden email]> wrote:
>
>> Hi,
>>
>> Can one of the committers please take a look at these PRs?
>>
>> https://github.com/apache/tomee/pull/377
>> https://github.com/apache/tomee/pull/376
>> https://github.com/apache/tomee/pull/375
>> https://github.com/apache/tomee/pull/370
>> https://github.com/apache/tomee/pull/366
>> https://github.com/apache/tomee/pull/363
>> https://github.com/apache/tomee/pull/362
>> https://github.com/apache/tomee/pull/361
>>
>> Cheers
>>
>> --
>> Bruno Baptista
>> https://twitter.com/brunobat_
>>
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: PR review and merge

Otávio Gonçalves de Santana
Thank you for the explanation it Roberto.

On Thu, Jan 24, 2019 at 12:50 PM Roberto Cortez <[hidden email]>
wrote:

> We didn’t.
>
> If we are moving forward with the release, I would prefer to keep these in
> stand by until we are done. Right now, we are on a green build, and we
> can’t be sure if any of these PR’s would break it, so better to be safe :)
>
> > On 24 Jan 2019, at 12:05, Otávio Gonçalves de Santana <
> [hidden email]> wrote:
> >
> > Hello everyone, please don't forget these PRs
> >
> > On Wed, Jan 23, 2019 at 8:01 AM Bruno Baptista <[hidden email]>
> wrote:
> >
> >> Hi,
> >>
> >> Can one of the committers please take a look at these PRs?
> >>
> >> https://github.com/apache/tomee/pull/377
> >> https://github.com/apache/tomee/pull/376
> >> https://github.com/apache/tomee/pull/375
> >> https://github.com/apache/tomee/pull/370
> >> https://github.com/apache/tomee/pull/366
> >> https://github.com/apache/tomee/pull/363
> >> https://github.com/apache/tomee/pull/362
> >> https://github.com/apache/tomee/pull/361
> >>
> >> Cheers
> >>
> >> --
> >> Bruno Baptista
> >> https://twitter.com/brunobat_
> >>
> >>
> >>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: PR review and merge

Roberto Cortez
I’ll be looking into the current list of PR’s and try to merge the ones that are ready.

Cheers,
Roberto

> On 24 Jan 2019, at 15:23, Otávio Gonçalves de Santana <[hidden email]> wrote:
>
> Thank you for the explanation it Roberto.
>
> On Thu, Jan 24, 2019 at 12:50 PM Roberto Cortez <[hidden email]>
> wrote:
>
>> We didn’t.
>>
>> If we are moving forward with the release, I would prefer to keep these in
>> stand by until we are done. Right now, we are on a green build, and we
>> can’t be sure if any of these PR’s would break it, so better to be safe :)
>>
>>> On 24 Jan 2019, at 12:05, Otávio Gonçalves de Santana <
>> [hidden email]> wrote:
>>>
>>> Hello everyone, please don't forget these PRs
>>>
>>> On Wed, Jan 23, 2019 at 8:01 AM Bruno Baptista <[hidden email]>
>> wrote:
>>>
>>>> Hi,
>>>>
>>>> Can one of the committers please take a look at these PRs?
>>>>
>>>> https://github.com/apache/tomee/pull/377
>>>> https://github.com/apache/tomee/pull/376
>>>> https://github.com/apache/tomee/pull/375
>>>> https://github.com/apache/tomee/pull/370
>>>> https://github.com/apache/tomee/pull/366
>>>> https://github.com/apache/tomee/pull/363
>>>> https://github.com/apache/tomee/pull/362
>>>> https://github.com/apache/tomee/pull/361
>>>>
>>>> Cheers
>>>>
>>>> --
>>>> Bruno Baptista
>>>> https://twitter.com/brunobat_
>>>>
>>>>
>>>>
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: PR review and merge

brunobat
Thanks Roberto.

Bruno Baptista
https://twitter.com/brunobat_


On 28/01/19 15:29, Roberto Cortez wrote:

> I’ll be looking into the current list of PR’s and try to merge the ones that are ready.
>
> Cheers,
> Roberto
>
>> On 24 Jan 2019, at 15:23, Otávio Gonçalves de Santana <[hidden email]> wrote:
>>
>> Thank you for the explanation it Roberto.
>>
>> On Thu, Jan 24, 2019 at 12:50 PM Roberto Cortez <[hidden email]>
>> wrote:
>>
>>> We didn’t.
>>>
>>> If we are moving forward with the release, I would prefer to keep these in
>>> stand by until we are done. Right now, we are on a green build, and we
>>> can’t be sure if any of these PR’s would break it, so better to be safe :)
>>>
>>>> On 24 Jan 2019, at 12:05, Otávio Gonçalves de Santana <
>>> [hidden email]> wrote:
>>>> Hello everyone, please don't forget these PRs
>>>>
>>>> On Wed, Jan 23, 2019 at 8:01 AM Bruno Baptista <[hidden email]>
>>> wrote:
>>>>> Hi,
>>>>>
>>>>> Can one of the committers please take a look at these PRs?
>>>>>
>>>>> https://github.com/apache/tomee/pull/377
>>>>> https://github.com/apache/tomee/pull/376
>>>>> https://github.com/apache/tomee/pull/375
>>>>> https://github.com/apache/tomee/pull/370
>>>>> https://github.com/apache/tomee/pull/366
>>>>> https://github.com/apache/tomee/pull/363
>>>>> https://github.com/apache/tomee/pull/362
>>>>> https://github.com/apache/tomee/pull/361
>>>>>
>>>>> Cheers
>>>>>
>>>>> --
>>>>> Bruno Baptista
>>>>> https://twitter.com/brunobat_
>>>>>
>>>>>
>>>>>
>>>
Reply | Threaded
Open this post in threaded view
|

Re: PR review and merge

Roberto Cortez
I think all of them are now merged, except for https://github.com/apache/tomee/pull/370 <https://github.com/apache/tomee/pull/370>

My concern here is we are changing the equals implementation, which is usually auto generated by our IDE’s. So, most likely these changes will be lost if someone regenerates the equals method, unless their IDE is set to use Objects.equals. This is no blocker, but I would like to discuss it further before moving forward with the PR.

Maybe we should force all .equals to use Objects.equals? With a checkstyle check (not sure if they have a check for that)?

Cheers,
Roberto

> On 28 Jan 2019, at 15:30, Bruno Baptista <[hidden email]> wrote:
>
> Thanks Roberto.
>
> Bruno Baptista
> https://twitter.com/brunobat_
>
>
> On 28/01/19 15:29, Roberto Cortez wrote:
>> I’ll be looking into the current list of PR’s and try to merge the ones that are ready.
>>
>> Cheers,
>> Roberto
>>
>>> On 24 Jan 2019, at 15:23, Otávio Gonçalves de Santana <[hidden email]> wrote:
>>>
>>> Thank you for the explanation it Roberto.
>>>
>>> On Thu, Jan 24, 2019 at 12:50 PM Roberto Cortez <[hidden email]>
>>> wrote:
>>>
>>>> We didn’t.
>>>>
>>>> If we are moving forward with the release, I would prefer to keep these in
>>>> stand by until we are done. Right now, we are on a green build, and we
>>>> can’t be sure if any of these PR’s would break it, so better to be safe :)
>>>>
>>>>> On 24 Jan 2019, at 12:05, Otávio Gonçalves de Santana <
>>>> [hidden email]> wrote:
>>>>> Hello everyone, please don't forget these PRs
>>>>>
>>>>> On Wed, Jan 23, 2019 at 8:01 AM Bruno Baptista <[hidden email]>
>>>> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Can one of the committers please take a look at these PRs?
>>>>>>
>>>>>> https://github.com/apache/tomee/pull/377
>>>>>> https://github.com/apache/tomee/pull/376
>>>>>> https://github.com/apache/tomee/pull/375
>>>>>> https://github.com/apache/tomee/pull/370
>>>>>> https://github.com/apache/tomee/pull/366
>>>>>> https://github.com/apache/tomee/pull/363
>>>>>> https://github.com/apache/tomee/pull/362
>>>>>> https://github.com/apache/tomee/pull/361
>>>>>>
>>>>>> Cheers
>>>>>>
>>>>>> --
>>>>>> Bruno Baptista
>>>>>> https://twitter.com/brunobat_
>>>>>>
>>>>>>
>>>>>>
>>>>

Reply | Threaded
Open this post in threaded view
|

Re: PR review and merge

brunobat
Thanks a lot Roberto!

Bruno Baptista
https://twitter.com/brunobat_


On 29/01/19 17:19, Roberto Cortez wrote:

> I think all of them are now merged, except for https://github.com/apache/tomee/pull/370 <https://github.com/apache/tomee/pull/370>
>
> My concern here is we are changing the equals implementation, which is usually auto generated by our IDE’s. So, most likely these changes will be lost if someone regenerates the equals method, unless their IDE is set to use Objects.equals. This is no blocker, but I would like to discuss it further before moving forward with the PR.
>
> Maybe we should force all .equals to use Objects.equals? With a checkstyle check (not sure if they have a check for that)?
>
> Cheers,
> Roberto
>
>> On 28 Jan 2019, at 15:30, Bruno Baptista <[hidden email]> wrote:
>>
>> Thanks Roberto.
>>
>> Bruno Baptista
>> https://twitter.com/brunobat_
>>
>>
>> On 28/01/19 15:29, Roberto Cortez wrote:
>>> I’ll be looking into the current list of PR’s and try to merge the ones that are ready.
>>>
>>> Cheers,
>>> Roberto
>>>
>>>> On 24 Jan 2019, at 15:23, Otávio Gonçalves de Santana <[hidden email]> wrote:
>>>>
>>>> Thank you for the explanation it Roberto.
>>>>
>>>> On Thu, Jan 24, 2019 at 12:50 PM Roberto Cortez <[hidden email]>
>>>> wrote:
>>>>
>>>>> We didn’t.
>>>>>
>>>>> If we are moving forward with the release, I would prefer to keep these in
>>>>> stand by until we are done. Right now, we are on a green build, and we
>>>>> can’t be sure if any of these PR’s would break it, so better to be safe :)
>>>>>
>>>>>> On 24 Jan 2019, at 12:05, Otávio Gonçalves de Santana <
>>>>> [hidden email]> wrote:
>>>>>> Hello everyone, please don't forget these PRs
>>>>>>
>>>>>> On Wed, Jan 23, 2019 at 8:01 AM Bruno Baptista <[hidden email]>
>>>>> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Can one of the committers please take a look at these PRs?
>>>>>>>
>>>>>>> https://github.com/apache/tomee/pull/377
>>>>>>> https://github.com/apache/tomee/pull/376
>>>>>>> https://github.com/apache/tomee/pull/375
>>>>>>> https://github.com/apache/tomee/pull/370
>>>>>>> https://github.com/apache/tomee/pull/366
>>>>>>> https://github.com/apache/tomee/pull/363
>>>>>>> https://github.com/apache/tomee/pull/362
>>>>>>> https://github.com/apache/tomee/pull/361
>>>>>>>
>>>>>>> Cheers
>>>>>>>
>>>>>>> --
>>>>>>> Bruno Baptista
>>>>>>> https://twitter.com/brunobat_
>>>>>>>
>>>>>>>
>>>>>>>
>