ofbiz-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Brohl <michael.br...@ecomify.de>
Subject Re: Removal of trailing white spaces, was: Re: [jira] [Commented] (OFBIZ-9777) [FB] Package org.apache.ofbiz.product.imagemanagement
Date Tue, 12 Dec 2017 12:29:40 GMT
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
>>>>
>>>>
>>>>
>>
>>
>
>



Mime
View raw message