You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Michael Brohl <mi...@ecomify.de> on 2017/12/12 09:27:55 UTC
Removal of trailing white spaces, was: Re: [jira] [Commented]
(OFBIZ-9777) [FB] Package org.apache.ofbiz.product.imagemanagement
Hi Jacques,
I propose to do it the other way around: when reviewing diffs, you can
configure the IDE to ignore these white space changes. It works
perfectly in Eclipse and you will not be bothered by such changes.
In my view, trailing white spaces do not belong there and should be
removed as part of the refactoring process. Else they would be there
forever.
To be precise, these are not false changes but intended changes to get
rid of trailing white spaces.
Thanks,
Michael
Am 12.12.17 um 10:04 schrieb Jacques Le Roux (JIRA):
> [ https://issues.apache.org/jira/browse/OFBIZ-9777?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16287290#comment-16287290 ]
>
> Jacques Le Roux commented on OFBIZ-9777:
> ----------------------------------------
>
> hanks Michael, Julian,
>
> Please Julian adjust your IDE or similar to not remove trailing white spaces when you create a patch, especially a big one, you may do that temporarily. Consider that else reviewers have to be confronted with a lot of false changes, thanks!
>
> BTW I have added a warning in https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices for that
>
>> [FB] Package org.apache.ofbiz.product.imagemanagement
>> -----------------------------------------------------
>>
>> Key: OFBIZ-9777
>> URL: https://issues.apache.org/jira/browse/OFBIZ-9777
>> Project: OFBiz
>> Issue Type: Sub-task
>> Components: product
>> Affects Versions: Trunk
>> Reporter: Julian Leichert
>> Assignee: Michael Brohl
>> Priority: Minor
>> Fix For: Upcoming Release
>>
>> Attachments: OFBIZ-9777_org.apache.ofbiz.product.imagemanagement_bugfixes.patch
>>
>>
>>
Re: Removal of trailing white spaces, was: Re: [jira] [Commented]
(OFBIZ-9777) [FB] Package org.apache.ofbiz.product.imagemanagement
Posted by Michael Brohl <mi...@ecomify.de>.
Am 12.12.17 um 11:48 schrieb Jacques Le Roux:
> This is a bit out of subject, but while at it:
> As you can see at [3] I'm not against removing trailing spaces. But
> experience told me one thing since. One of the most important things
> in version control is what happened to a line, because you will need
> to track issues in the past. This with svn (harder but possible with
> Git) is done with annotations (aka blame). When we make batch changes
> (like I had to do for definitely resolving the EOLs issue in SVN) you
> lose this information and it's unfortunate. That's another good reason
> to not mix formatting changes with functional changes.
One additional remark: the refactoring tickets only deal with
formatting, they contain no functional changes (or should not contain
functional changes). That's why we have provided the FB issues first and
after they were committed, do the refactoring on base of this.
So we should be on the right track I guess.
Michael
Re: Removal of trailing white spaces, was: Re: [jira] [Commented]
(OFBIZ-9777) [FB] Package org.apache.ofbiz.product.imagemanagement
Posted by Michael Brohl <mi...@ecomify.de>.
Am 12.12.17 um 14:38 schrieb Jacques Le Roux:
> I catch up to Jacques's comment, I prefer to separate the formating to
> the functional even if I was do the same in the past and I will do it
> by oversight in the future :)
> This was what David taught us and I found it reasonable
>
Yes, and that's exactly what we did: FindBugs first,
refactoring/reformatting second. See my previous mail on this topic.
I think that we are not too much away from each other...
Re: Removal of trailing white spaces, was: Re: [jira] [Commented]
(OFBIZ-9777) [FB] Package org.apache.ofbiz.product.imagemanagement
Posted by Jacques Le Roux <ja...@les7arts.com>.
Le 12/12/2017 à 14:13, Nicolas Malin a écrit :
> Hello
>
> Le 12/12/2017 à 13:29, Michael Brohl a écrit :
>> [...] I have a very simple but effective workflow for this, you might find it helpful also:
>>
>> 1. copy the link to the patch file in the Jira issue
>> 2. right click on your OFBiz project in Eclipse and select Team/Apply patch
>> 3. the copied link should be filled in the Url selection, just click next
>> 4. select the project to apply the patch. It should be pre-selected so just click next
>>
>> You now get a nice view with all changes and a graphical diff to review the changes. If there are conflicts (e.g. the patch is outdated), you can
>> resolve them and change the patch on the fly.
> Oh, it's not easier for all, like me, too long, I use directly emacs
>>> So, I'm reviewing directly in my email clients, and IDE is not relevant for me. Maybe a solution would be to suggest another consensual reviewing
>>> process, there are tools for that[2]. And then possibly removing trailing spaces would be a good thing indeed.
>> Of course it's a little difficult when you are just looking at plain text. You can use a trick: just mark the diff part where you are searching for
>> a change and you'll see the trailing whitespace.
>>
>> Personally, I cannot imagine to review patches in an email client without having a look at the context (code around the diffs). This might work for
>> very simple things but definitely not for more comprehensive patches.
> Damned, I'm an young old man ? because I review 9 patch / 10 directly on the email content, If I have a doubt, switch to emacs and if I no
> understand more switch to intellij
Ah, so I'm not alone, even if it's not the same tools ;)
> I catch up to Jacques's comment, I prefer to separate the formating to the functional even if I was do the same in the past and I will do it by
> oversight in the future :)
This was what David taught us and I found it reasonable
Thanks Nicolas
Jacques
>
> Nicolas
>>
>>>
>>> Jacques
>>> [0] http://markmail.org/message/mdiokme77l3v2sja
>>> [1] http://markmail.org/message/lbwz5puicmaleb7s
>>> [2] https://en.wikipedia.org/wiki/List_of_tools_for_code_review
>>> [3] http://markmail.org/message/ir7cs2ofbzbzc53a
>>>
>>>
>>> Le 12/12/2017 à 10:27, Michael Brohl a écrit :
>>>> Hi Jacques,
>>>>
>>>> I propose to do it the other way around: when reviewing diffs, you can configure the IDE to ignore these white space changes. It works perfectly
>>>> in Eclipse and you will not be bothered by such changes.
>>>>
>>>> In my view, trailing white spaces do not belong there and should be removed as part of the refactoring process. Else they would be there forever.
>>>>
>>>> To be precise, these are not false changes but intended changes to get rid of trailing white spaces.
>>>>
>>>> Thanks,
>>>>
>>>> Michael
>>>>
>>>>
>>>> Am 12.12.17 um 10:04 schrieb Jacques Le Roux (JIRA):
>>>>> [
>>>>> https://issues.apache.org/jira/browse/OFBIZ-9777?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16287290#comment-16287290
>>>>> ]
>>>>>
>>>>> Jacques Le Roux commented on OFBIZ-9777:
>>>>> ----------------------------------------
>>>>>
>>>>> hanks Michael, Julian,
>>>>>
>>>>> Please Julian adjust your IDE or similar to not remove trailing white spaces when you create a patch, especially a big one, you may do that
>>>>> temporarily. Consider that else reviewers have to be confronted with a lot of false changes, thanks!
>>>>>
>>>>> BTW I have added a warning in https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices for that
>>>>>
>>>>>> [FB] Package org.apache.ofbiz.product.imagemanagement
>>>>>> -----------------------------------------------------
>>>>>>
>>>>>> Key: OFBIZ-9777
>>>>>> URL: https://issues.apache.org/jira/browse/OFBIZ-9777
>>>>>> Project: OFBiz
>>>>>> Issue Type: Sub-task
>>>>>> Components: product
>>>>>> Affects Versions: Trunk
>>>>>> Reporter: Julian Leichert
>>>>>> Assignee: Michael Brohl
>>>>>> Priority: Minor
>>>>>> Fix For: Upcoming Release
>>>>>>
>>>>>> Attachments: OFBIZ-9777_org.apache.ofbiz.product.imagemanagement_bugfixes.patch
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
Re: Removal of trailing white spaces, was: Re: [jira] [Commented]
(OFBIZ-9777) [FB] Package org.apache.ofbiz.product.imagemanagement
Posted by Nicolas Malin <ni...@nereide.fr>.
Hello
Le 12/12/2017 à 13:29, Michael Brohl a écrit :
> [...] I have a very simple but effective workflow for this, you might
> find it helpful also:
>
> 1. copy the link to the patch file in the Jira issue
> 2. right click on your OFBiz project in Eclipse and select Team/Apply
> patch
> 3. the copied link should be filled in the Url selection, just click next
> 4. select the project to apply the patch. It should be pre-selected so
> just click next
>
> You now get a nice view with all changes and a graphical diff to
> review the changes. If there are conflicts (e.g. the patch is
> outdated), you can resolve them and change the patch on the fly.
Oh, it's not easier for all, like me, too long, I use directly emacs
>> So, I'm reviewing directly in my email clients, and IDE is not
>> relevant for me. Maybe a solution would be to suggest another
>> consensual reviewing process, there are tools for that[2]. And then
>> possibly removing trailing spaces would be a good thing indeed.
> Of course it's a little difficult when you are just looking at plain
> text. You can use a trick: just mark the diff part where you are
> searching for a change and you'll see the trailing whitespace.
>
> Personally, I cannot imagine to review patches in an email client
> without having a look at the context (code around the diffs). This
> might work for very simple things but definitely not for more
> comprehensive patches.
Damned, I'm an young old man ? because I review 9 patch / 10 directly on
the email content, If I have a doubt, switch to emacs and if I no
understand more switch to intellij
I catch up to Jacques's comment, I prefer to separate the formating to
the functional even if I was do the same in the past and I will do it by
oversight in the future :)
Nicolas
>
>>
>> Jacques
>> [0] http://markmail.org/message/mdiokme77l3v2sja
>> [1] http://markmail.org/message/lbwz5puicmaleb7s
>> [2] https://en.wikipedia.org/wiki/List_of_tools_for_code_review
>> [3] http://markmail.org/message/ir7cs2ofbzbzc53a
>>
>>
>> Le 12/12/2017 à 10:27, Michael Brohl a écrit :
>>> Hi Jacques,
>>>
>>> I propose to do it the other way around: when reviewing diffs, you
>>> can configure the IDE to ignore these white space changes. It works
>>> perfectly in Eclipse and you will not be bothered by such changes.
>>>
>>> In my view, trailing white spaces do not belong there and should be
>>> removed as part of the refactoring process. Else they would be there
>>> forever.
>>>
>>> To be precise, these are not false changes but intended changes to
>>> get rid of trailing white spaces.
>>>
>>> Thanks,
>>>
>>> Michael
>>>
>>>
>>> Am 12.12.17 um 10:04 schrieb Jacques Le Roux (JIRA):
>>>> [
>>>> https://issues.apache.org/jira/browse/OFBIZ-9777?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16287290#comment-16287290
>>>> ]
>>>>
>>>> Jacques Le Roux commented on OFBIZ-9777:
>>>> ----------------------------------------
>>>>
>>>> hanks Michael, Julian,
>>>>
>>>> Please Julian adjust your IDE or similar to not remove trailing
>>>> white spaces when you create a patch, especially a big one, you may
>>>> do that temporarily. Consider that else reviewers have to be
>>>> confronted with a lot of false changes, thanks!
>>>>
>>>> BTW I have added a warning in
>>>> https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices
>>>> for that
>>>>
>>>>> [FB] Package org.apache.ofbiz.product.imagemanagement
>>>>> -----------------------------------------------------
>>>>>
>>>>> Key: OFBIZ-9777
>>>>> URL:
>>>>> https://issues.apache.org/jira/browse/OFBIZ-9777
>>>>> Project: OFBiz
>>>>> Issue Type: Sub-task
>>>>> Components: product
>>>>> Affects Versions: Trunk
>>>>> Reporter: Julian Leichert
>>>>> Assignee: Michael Brohl
>>>>> Priority: Minor
>>>>> Fix For: Upcoming Release
>>>>>
>>>>> Attachments:
>>>>> OFBIZ-9777_org.apache.ofbiz.product.imagemanagement_bugfixes.patch
>>>>>
>>>>>
>>>>>
>>>
>>>
>>
>>
>
>
Re: Removal of trailing white spaces, was: Re: [jira] [Commented]
(OFBIZ-9777) [FB] Package org.apache.ofbiz.product.imagemanagement
Posted by Jacques Le Roux <ja...@les7arts.com>.
BTW Michael,
Don't get me wrong, I want to again compliment you and your team for the excellent work done recently :)
Jacques
Le 12/12/2017 à 14:28, Jacques Le Roux a écrit :
> Le 12/12/2017 à 13:29, Michael Brohl a écrit :
>> Hi Jacques,
>>
>> inline...
>>
>> Am 12.12.17 um 11:48 schrieb Jacques Le Roux:
>>> Hi Michael,
>>>
>>> I already answered you in Jira here it is another more complete version:
>>>
>>> Removing trailing spaces has been discussed in the past and the consensus is that we don't remove trailing spaces in patches because it hurts
>>> reviewers with false changes.
>> In my view, these are not false changes. They are intended changes which are clearly visible when you use proper tools.
>>
>>>
>>> What was suggested then is to split formatting changes from functional changes when creating patches, here is an example of this suggestion[1]. At
>>> least we could split trailing spaces removing patches they can sometimes be really a pain! As I said in the Jira another solution would be to use
>>> a reviewing tool. I tried https://reviews.apache.org in the past[0] but was finally not satisfied and got back to my email client.
>> I have a very simple but effective workflow for this, you might find it helpful also:
>>
>> 1. copy the link to the patch file in the Jira issue
>> 2. right click on your OFBiz project in Eclipse and select Team/Apply patch
>> 3. the copied link should be filled in the Url selection, just click next
>> 4. select the project to apply the patch. It should be pre-selected so just click next
>>
>> You now get a nice view with all changes and a graphical diff to review the changes. If there are conflicts (e.g. the patch is outdated), you can
>> resolve them and change the patch on the fly.
> Yes this is what I use when patching. But I speak about (a lot of) patches already committed and updated on my machine.
>
>>
>>>
>>> So, I'm reviewing directly in my email clients, and IDE is not relevant for me. Maybe a solution would be to suggest another consensual reviewing
>>> process, there are tools for that[2]. And then possibly removing trailing spaces would be a good thing indeed.
>> Of course it's a little difficult when you are just looking at plain text. You can use a trick: just mark the diff part where you are searching for
>> a change and you'll see the trailing whitespace.
> I see the removed trailing white spaces, it's obvious: same lines. The problem is when you have much in a patch it makes the review longer because
> you check things which are not real changes. And when you get a burst of patches (I speak about 73 of yours in 4 days, I have still 14 to review)
> then things get more complicated. I never love to review burst of patches anyway. I understand it's sometimes difficult to make otherwise. I though
> still recommend the 10 minutes way: http://markmail.org/message/ncry5jsadflib7do. I mean less at the "same" time and more often some...
>
>>
>> Personally, I cannot imagine to review patches in an email client without having a look at the context (code around the diffs). This might work for
>> very simple things but definitely not for more comprehensive patches.
> Once committed and updated it's a bit more complicate. Of course, there are other process flows that I could use. Like reverting, and applying
> patches as you said above. But see I'm then reviewing a lot of patches, w/ some large, a the same time and that's why I use the email client. When I
> need, mostly when patching, I get to Eclipse and even use WinMerge as a plus. It's a marvellous tool for local review, better than Eclipse at least.
>
>> I think we should get rid of the trailing whitespaces because they don't belong to the code and they are the root cause why we are discussing this.
>> We use tools to automatically remove them when saving files. How should we get rid of them otherwise?
> It seems nobody is against apart me now, so let it be...
>
> Jacques
>
>>
>> It's definetely not an option for us to do it manually and provide separate patches for it. We can just switch off the feature and leave them in,
>> but that does not solve the root problem.
>>
>> The heavy refactoring work were we are touching many files is, in my view, the best chance to get rid of the trailing whitespaces now.
>> Having some occasional changes which are not visible in plain text should not block this effort.
>>
>>> This is a bit out of subject, but while at it:
>>> As you can see at [3] I'm not against removing trailing spaces. But experience told me one thing since. One of the most important things in
>>> version control is what happened to a line, because you will need to track issues in the past. This with svn (harder but possible with Git) is
>>> done with annotations (aka blame). When we make batch changes (like I had to do for definitely resolving the EOLs issue in SVN) you lose this
>>> information and it's unfortunate. That's another good reason to not mix formatting changes with functional changes.
>> I see your point but we should see this in proper relation: we don't have massive trailing white space changes in the patches. It's not an issue
>> when we lose some line related info here and there. It's completely different from doing a mass change of EOL formatting where every line is affected.
>>
>> Thanks,
>> Michael
>>
>>
>>>
>>> Jacques
>>> [0] http://markmail.org/message/mdiokme77l3v2sja
>>> [1] http://markmail.org/message/lbwz5puicmaleb7s
>>> [2] https://en.wikipedia.org/wiki/List_of_tools_for_code_review
>>> [3] http://markmail.org/message/ir7cs2ofbzbzc53a
>>>
>>>
>>> Le 12/12/2017 à 10:27, Michael Brohl a écrit :
>>>> Hi Jacques,
>>>>
>>>> I propose to do it the other way around: when reviewing diffs, you can configure the IDE to ignore these white space changes. It works perfectly
>>>> in Eclipse and you will not be bothered by such changes.
>>>>
>>>> In my view, trailing white spaces do not belong there and should be removed as part of the refactoring process. Else they would be there forever.
>>>>
>>>> To be precise, these are not false changes but intended changes to get rid of trailing white spaces.
>>>>
>>>> Thanks,
>>>>
>>>> Michael
>>>>
>>>>
>>>> Am 12.12.17 um 10:04 schrieb Jacques Le Roux (JIRA):
>>>>> [
>>>>> https://issues.apache.org/jira/browse/OFBIZ-9777?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16287290#comment-16287290
>>>>> ]
>>>>>
>>>>> Jacques Le Roux commented on OFBIZ-9777:
>>>>> ----------------------------------------
>>>>>
>>>>> hanks Michael, Julian,
>>>>>
>>>>> Please Julian adjust your IDE or similar to not remove trailing white spaces when you create a patch, especially a big one, you may do that
>>>>> temporarily. Consider that else reviewers have to be confronted with a lot of false changes, thanks!
>>>>>
>>>>> BTW I have added a warning in https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices for that
>>>>>
>>>>>> [FB] Package org.apache.ofbiz.product.imagemanagement
>>>>>> -----------------------------------------------------
>>>>>>
>>>>>> Key: OFBIZ-9777
>>>>>> URL: https://issues.apache.org/jira/browse/OFBIZ-9777
>>>>>> Project: OFBiz
>>>>>> Issue Type: Sub-task
>>>>>> Components: product
>>>>>> Affects Versions: Trunk
>>>>>> Reporter: Julian Leichert
>>>>>> Assignee: Michael Brohl
>>>>>> Priority: Minor
>>>>>> Fix For: Upcoming Release
>>>>>>
>>>>>> Attachments: OFBIZ-9777_org.apache.ofbiz.product.imagemanagement_bugfixes.patch
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
Re: Removal of trailing white spaces, was: Re: [jira] [Commented]
(OFBIZ-9777) [FB] Package org.apache.ofbiz.product.imagemanagement
Posted by Jacques Le Roux <ja...@les7arts.com>.
Le 12/12/2017 à 13:29, Michael Brohl a écrit :
> Hi Jacques,
>
> inline...
>
> Am 12.12.17 um 11:48 schrieb Jacques Le Roux:
>> Hi Michael,
>>
>> I already answered you in Jira here it is another more complete version:
>>
>> Removing trailing spaces has been discussed in the past and the consensus is that we don't remove trailing spaces in patches because it hurts
>> reviewers with false changes.
> In my view, these are not false changes. They are intended changes which are clearly visible when you use proper tools.
>
>>
>> What was suggested then is to split formatting changes from functional changes when creating patches, here is an example of this suggestion[1]. At
>> least we could split trailing spaces removing patches they can sometimes be really a pain! As I said in the Jira another solution would be to use a
>> reviewing tool. I tried https://reviews.apache.org in the past[0] but was finally not satisfied and got back to my email client.
> I have a very simple but effective workflow for this, you might find it helpful also:
>
> 1. copy the link to the patch file in the Jira issue
> 2. right click on your OFBiz project in Eclipse and select Team/Apply patch
> 3. the copied link should be filled in the Url selection, just click next
> 4. select the project to apply the patch. It should be pre-selected so just click next
>
> You now get a nice view with all changes and a graphical diff to review the changes. If there are conflicts (e.g. the patch is outdated), you can
> resolve them and change the patch on the fly.
Yes this is what I use when patching. But I speak about (a lot of) patches already committed and updated on my machine.
>
>>
>> So, I'm reviewing directly in my email clients, and IDE is not relevant for me. Maybe a solution would be to suggest another consensual reviewing
>> process, there are tools for that[2]. And then possibly removing trailing spaces would be a good thing indeed.
> Of course it's a little difficult when you are just looking at plain text. You can use a trick: just mark the diff part where you are searching for
> a change and you'll see the trailing whitespace.
I see the removed trailing white spaces, it's obvious: same lines. The problem is when you have much in a patch it makes the review longer because you
check things which are not real changes. And when you get a burst of patches (I speak about 73 of yours in 4 days, I have still 14 to review) then
things get more complicated. I never love to review burst of patches anyway. I understand it's sometimes difficult to make otherwise. I though still
recommend the 10 minutes way: http://markmail.org/message/ncry5jsadflib7do. I mean less at the "same" time and more often some...
>
> Personally, I cannot imagine to review patches in an email client without having a look at the context (code around the diffs). This might work for
> very simple things but definitely not for more comprehensive patches.
Once committed and updated it's a bit more complicate. Of course, there are other process flows that I could use. Like reverting, and applying patches
as you said above. But see I'm then reviewing a lot of patches, w/ some large, a the same time and that's why I use the email client. When I need,
mostly when patching, I get to Eclipse and even use WinMerge as a plus. It's a marvellous tool for local review, better than Eclipse at least.
> I think we should get rid of the trailing whitespaces because they don't belong to the code and they are the root cause why we are discussing this.
> We use tools to automatically remove them when saving files. How should we get rid of them otherwise?
It seems nobody is against apart me now, so let it be...
Jacques
>
> It's definetely not an option for us to do it manually and provide separate patches for it. We can just switch off the feature and leave them in,
> but that does not solve the root problem.
>
> The heavy refactoring work were we are touching many files is, in my view, the best chance to get rid of the trailing whitespaces now.
> Having some occasional changes which are not visible in plain text should not block this effort.
>
>> This is a bit out of subject, but while at it:
>> As you can see at [3] I'm not against removing trailing spaces. But experience told me one thing since. One of the most important things in
>> version control is what happened to a line, because you will need to track issues in the past. This with svn (harder but possible with Git) is
>> done with annotations (aka blame). When we make batch changes (like I had to do for definitely resolving the EOLs issue in SVN) you lose this
>> information and it's unfortunate. That's another good reason to not mix formatting changes with functional changes.
> I see your point but we should see this in proper relation: we don't have massive trailing white space changes in the patches. It's not an issue
> when we lose some line related info here and there. It's completely different from doing a mass change of EOL formatting where every line is affected.
>
> Thanks,
> Michael
>
>
>>
>> Jacques
>> [0] http://markmail.org/message/mdiokme77l3v2sja
>> [1] http://markmail.org/message/lbwz5puicmaleb7s
>> [2] https://en.wikipedia.org/wiki/List_of_tools_for_code_review
>> [3] http://markmail.org/message/ir7cs2ofbzbzc53a
>>
>>
>> Le 12/12/2017 à 10:27, Michael Brohl a écrit :
>>> Hi Jacques,
>>>
>>> I propose to do it the other way around: when reviewing diffs, you can configure the IDE to ignore these white space changes. It works perfectly
>>> in Eclipse and you will not be bothered by such changes.
>>>
>>> In my view, trailing white spaces do not belong there and should be removed as part of the refactoring process. Else they would be there forever.
>>>
>>> To be precise, these are not false changes but intended changes to get rid of trailing white spaces.
>>>
>>> Thanks,
>>>
>>> Michael
>>>
>>>
>>> Am 12.12.17 um 10:04 schrieb Jacques Le Roux (JIRA):
>>>> [
>>>> https://issues.apache.org/jira/browse/OFBIZ-9777?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16287290#comment-16287290
>>>> ]
>>>>
>>>> Jacques Le Roux commented on OFBIZ-9777:
>>>> ----------------------------------------
>>>>
>>>> hanks Michael, Julian,
>>>>
>>>> Please Julian adjust your IDE or similar to not remove trailing white spaces when you create a patch, especially a big one, you may do that
>>>> temporarily. Consider that else reviewers have to be confronted with a lot of false changes, thanks!
>>>>
>>>> BTW I have added a warning in https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices for that
>>>>
>>>>> [FB] Package org.apache.ofbiz.product.imagemanagement
>>>>> -----------------------------------------------------
>>>>>
>>>>> Key: OFBIZ-9777
>>>>> URL: https://issues.apache.org/jira/browse/OFBIZ-9777
>>>>> Project: OFBiz
>>>>> Issue Type: Sub-task
>>>>> Components: product
>>>>> Affects Versions: Trunk
>>>>> Reporter: Julian Leichert
>>>>> Assignee: Michael Brohl
>>>>> Priority: Minor
>>>>> Fix For: Upcoming Release
>>>>>
>>>>> Attachments: OFBIZ-9777_org.apache.ofbiz.product.imagemanagement_bugfixes.patch
>>>>>
>>>>>
>>>>>
>>>
>>>
>>
>>
>
>
Re: Removal of trailing white spaces, was: Re: [jira] [Commented]
(OFBIZ-9777) [FB] Package org.apache.ofbiz.product.imagemanagement
Posted by Michael Brohl <mi...@ecomify.de>.
Hi Jacques,
inline...
Am 12.12.17 um 11:48 schrieb Jacques Le Roux:
> Hi Michael,
>
> I already answered you in Jira here it is another more complete version:
>
> Removing trailing spaces has been discussed in the past and the
> consensus is that we don't remove trailing spaces in patches because
> it hurts reviewers with false changes.
In my view, these are not false changes. They are intended changes which
are clearly visible when you use proper tools.
>
> What was suggested then is to split formatting changes from functional
> changes when creating patches, here is an example of this
> suggestion[1]. At least we could split trailing spaces removing
> patches they can sometimes be really a pain! As I said in the Jira
> another solution would be to use a reviewing tool. I tried
> https://reviews.apache.org in the past[0] but was finally not
> satisfied and got back to my email client.
I have a very simple but effective workflow for this, you might find it
helpful also:
1. copy the link to the patch file in the Jira issue
2. right click on your OFBiz project in Eclipse and select Team/Apply patch
3. the copied link should be filled in the Url selection, just click next
4. select the project to apply the patch. It should be pre-selected so
just click next
You now get a nice view with all changes and a graphical diff to review
the changes. If there are conflicts (e.g. the patch is outdated), you
can resolve them and change the patch on the fly.
>
> So, I'm reviewing directly in my email clients, and IDE is not
> relevant for me. Maybe a solution would be to suggest another
> consensual reviewing process, there are tools for that[2]. And then
> possibly removing trailing spaces would be a good thing indeed.
Of course it's a little difficult when you are just looking at plain
text. You can use a trick: just mark the diff part where you are
searching for a change and you'll see the trailing whitespace.
Personally, I cannot imagine to review patches in an email client
without having a look at the context (code around the diffs). This might
work for very simple things but definitely not for more comprehensive
patches.
I think we should get rid of the trailing whitespaces because they don't
belong to the code and they are the root cause why we are discussing this.
We use tools to automatically remove them when saving files. How should
we get rid of them otherwise?
It's definetely not an option for us to do it manually and provide
separate patches for it. We can just switch off the feature and leave
them in, but that does not solve the root problem.
The heavy refactoring work were we are touching many files is, in my
view, the best chance to get rid of the trailing whitespaces now.
Having some occasional changes which are not visible in plain text
should not block this effort.
> This is a bit out of subject, but while at it:
> As you can see at [3] I'm not against removing trailing spaces. But
> experience told me one thing since. One of the most important things
> in version control is what happened to a line, because you will need
> to track issues in the past. This with svn (harder but possible with
> Git) is done with annotations (aka blame). When we make batch changes
> (like I had to do for definitely resolving the EOLs issue in SVN) you
> lose this information and it's unfortunate. That's another good reason
> to not mix formatting changes with functional changes.
I see your point but we should see this in proper relation: we don't
have massive trailing white space changes in the patches. It's not an
issue when we lose some line related info here and there. It's
completely different from doing a mass change of EOL formatting where
every line is affected.
Thanks,
Michael
>
> Jacques
> [0] http://markmail.org/message/mdiokme77l3v2sja
> [1] http://markmail.org/message/lbwz5puicmaleb7s
> [2] https://en.wikipedia.org/wiki/List_of_tools_for_code_review
> [3] http://markmail.org/message/ir7cs2ofbzbzc53a
>
>
> Le 12/12/2017 à 10:27, Michael Brohl a écrit :
>> Hi Jacques,
>>
>> I propose to do it the other way around: when reviewing diffs, you
>> can configure the IDE to ignore these white space changes. It works
>> perfectly in Eclipse and you will not be bothered by such changes.
>>
>> In my view, trailing white spaces do not belong there and should be
>> removed as part of the refactoring process. Else they would be there
>> forever.
>>
>> To be precise, these are not false changes but intended changes to
>> get rid of trailing white spaces.
>>
>> Thanks,
>>
>> Michael
>>
>>
>> Am 12.12.17 um 10:04 schrieb Jacques Le Roux (JIRA):
>>> [
>>> https://issues.apache.org/jira/browse/OFBIZ-9777?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16287290#comment-16287290
>>> ]
>>>
>>> Jacques Le Roux commented on OFBIZ-9777:
>>> ----------------------------------------
>>>
>>> hanks Michael, Julian,
>>>
>>> Please Julian adjust your IDE or similar to not remove trailing
>>> white spaces when you create a patch, especially a big one, you may
>>> do that temporarily. Consider that else reviewers have to be
>>> confronted with a lot of false changes, thanks!
>>>
>>> BTW I have added a warning in
>>> https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices
>>> for that
>>>
>>>> [FB] Package org.apache.ofbiz.product.imagemanagement
>>>> -----------------------------------------------------
>>>>
>>>> Key: OFBIZ-9777
>>>> URL: https://issues.apache.org/jira/browse/OFBIZ-9777
>>>> Project: OFBiz
>>>> Issue Type: Sub-task
>>>> Components: product
>>>> Affects Versions: Trunk
>>>> Reporter: Julian Leichert
>>>> Assignee: Michael Brohl
>>>> Priority: Minor
>>>> Fix For: Upcoming Release
>>>>
>>>> Attachments:
>>>> OFBIZ-9777_org.apache.ofbiz.product.imagemanagement_bugfixes.patch
>>>>
>>>>
>>>>
>>
>>
>
>
Re: Removal of trailing white spaces, was: Re: [jira] [Commented]
(OFBIZ-9777) [FB] Package org.apache.ofbiz.product.imagemanagement
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi Michael,
I already answered you in Jira here it is another more complete version:
Removing trailing spaces has been discussed in the past and the consensus is that we don't remove trailing spaces in patches because it hurts
reviewers with false changes.
What was suggested then is to split formatting changes from functional changes when creating patches, here is an example of this suggestion[1]. At
least we could split trailing spaces removing patches they can sometimes be really a pain! As I said in the Jira another solution would be to use a
reviewing tool. I tried https://reviews.apache.org in the past[0] but was finally not satisfied and got back to my email client.
So, I'm reviewing directly in my email clients, and IDE is not relevant for me. Maybe a solution would be to suggest another consensual reviewing
process, there are tools for that[2]. And then possibly removing trailing spaces would be a good thing indeed.
This is a bit out of subject, but while at it:
As you can see at [3] I'm not against removing trailing spaces. But experience told me one thing since. One of the most important things in version
control is what happened to a line, because you will need to track issues in the past. This with svn (harder but possible with Git) is done with
annotations (aka blame). When we make batch changes (like I had to do for definitely resolving the EOLs issue in SVN) you lose this information and
it's unfortunate. That's another good reason to not mix formatting changes with functional changes.
Jacques
[0] http://markmail.org/message/mdiokme77l3v2sja
[1] http://markmail.org/message/lbwz5puicmaleb7s
[2] https://en.wikipedia.org/wiki/List_of_tools_for_code_review
[3] http://markmail.org/message/ir7cs2ofbzbzc53a
Le 12/12/2017 à 10:27, Michael Brohl a écrit :
> Hi Jacques,
>
> I propose to do it the other way around: when reviewing diffs, you can configure the IDE to ignore these white space changes. It works perfectly in
> Eclipse and you will not be bothered by such changes.
>
> In my view, trailing white spaces do not belong there and should be removed as part of the refactoring process. Else they would be there forever.
>
> To be precise, these are not false changes but intended changes to get rid of trailing white spaces.
>
> Thanks,
>
> Michael
>
>
> Am 12.12.17 um 10:04 schrieb Jacques Le Roux (JIRA):
>> [
>> https://issues.apache.org/jira/browse/OFBIZ-9777?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16287290#comment-16287290
>> ]
>>
>> Jacques Le Roux commented on OFBIZ-9777:
>> ----------------------------------------
>>
>> hanks Michael, Julian,
>>
>> Please Julian adjust your IDE or similar to not remove trailing white spaces when you create a patch, especially a big one, you may do that
>> temporarily. Consider that else reviewers have to be confronted with a lot of false changes, thanks!
>>
>> BTW I have added a warning in https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices for that
>>
>>> [FB] Package org.apache.ofbiz.product.imagemanagement
>>> -----------------------------------------------------
>>>
>>> Key: OFBIZ-9777
>>> URL: https://issues.apache.org/jira/browse/OFBIZ-9777
>>> Project: OFBiz
>>> Issue Type: Sub-task
>>> Components: product
>>> Affects Versions: Trunk
>>> Reporter: Julian Leichert
>>> Assignee: Michael Brohl
>>> Priority: Minor
>>> Fix For: Upcoming Release
>>>
>>> Attachments: OFBIZ-9777_org.apache.ofbiz.product.imagemanagement_bugfixes.patch
>>>
>>>
>>>
>
>