Re: svn commit: r1844729 - in /ofbiz/ofbiz-framework/trunk/framework/webapp: dtd/site-conf.xsd src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java

[Mathieu Lirzin]

Hello,

[hidden email] writes:

> Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java?rev=1844729&r1=1844728&r2=1844729&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java (original)
> +++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java Wed Oct 24 07:55:37 2018
> @@ -695,6 +695,10 @@ public class RequestHandler {
>              if ("url".equals(nextRequestResponse.type)) {
>                  if (Debug.verboseOn()) Debug.logVerbose("[RequestHandler.doRequest]: Response is a URL redirect." + showSessionId(request), module);
>                  callRedirect(nextRequestResponse.value, response, request, ccfg.getStatusCodeString());
> +            } else if ("url-redirect".equals(nextRequestResponse.type)) {
> +                // check for a cross-application redirect
> +                if (Debug.verboseOn()) Debug.logVerbose("[RequestHandler.doRequest]: Response is a URL redirect with redirect parameters." + showSessionId(request), module);
> +                callRedirect(nextRequestResponse.value + this.makeQueryString(request, nextRequestResponse), response, request, ccfg.getStatusCodeString());
>              } else if ("cross-redirect".equals(nextRequestResponse.type)) {
>                  // check for a cross-application redirect
>                  if (Debug.verboseOn()) Debug.logVerbose("[RequestHandler.doRequest]: Response is a Cross-Application redirect." + showSessionId(request), module);

I have a just bought a huge 4K screen so I think it should be formatted
this way instead:

--8<---------------cut here---------------start------------->8---
// check for a cross-application redirect
else if ("url-redirect".equals(nextRequestResponse.type)) { if (Debug.verboseOn()) Debug.logVerbose("[RequestHandler.doRequest]: Response is a URL redirect with redirect parameters." + showSessionId(request), module); callRedirect(nextRequestResponse.value + this.makeQueryString(request, nextRequestResponse), response, request, ccfg.getStatusCodeString()); }
--8<---------------cut here---------------end--------------->8---

Joke aside :-), it would be *really* great if people could make an
effort sticking to the convention of using no more than 120 characters
per line which is already enough to make my eyes bleed.

Thanks.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Jacques Le Roux]

Hi Mathieu,

Yes you are right, the whole file should be reformatted.

This "around 120 chars max" rule is "new" (few years) and most of the code there is more than a decade.

If nobody disagree we could have a task Jira to reformat the code of the most important classes (with subtasks maybe, or simply patches for concerned
classes).

Jacques


Le 25/10/2018 à 22:50, Mathieu Lirzin a écrit :

> Hello,
>
> [hidden email] writes:
>
>> Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java?rev=1844729&r1=1844728&r2=1844729&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java (original)
>> +++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java Wed Oct 24 07:55:37 2018
>> @@ -695,6 +695,10 @@ public class RequestHandler {
>>               if ("url".equals(nextRequestResponse.type)) {
>>                   if (Debug.verboseOn()) Debug.logVerbose("[RequestHandler.doRequest]: Response is a URL redirect." + showSessionId(request), module);
>>                   callRedirect(nextRequestResponse.value, response, request, ccfg.getStatusCodeString());
>> +            } else if ("url-redirect".equals(nextRequestResponse.type)) {
>> +                // check for a cross-application redirect
>> +                if (Debug.verboseOn()) Debug.logVerbose("[RequestHandler.doRequest]: Response is a URL redirect with redirect parameters." + showSessionId(request), module);
>> +                callRedirect(nextRequestResponse.value + this.makeQueryString(request, nextRequestResponse), response, request, ccfg.getStatusCodeString());
>>               } else if ("cross-redirect".equals(nextRequestResponse.type)) {
>>                   // check for a cross-application redirect
>>                   if (Debug.verboseOn()) Debug.logVerbose("[RequestHandler.doRequest]: Response is a Cross-Application redirect." + showSessionId(request), module);
> I have a just bought a huge 4K screen so I think it should be formatted
> this way instead:
>
> --8<---------------cut here---------------start------------->8---
> // check for a cross-application redirect
> else if ("url-redirect".equals(nextRequestResponse.type)) { if (Debug.verboseOn()) Debug.logVerbose("[RequestHandler.doRequest]: Response is a URL redirect with redirect parameters." + showSessionId(request), module); callRedirect(nextRequestResponse.value + this.makeQueryString(request, nextRequestResponse), response, request, ccfg.getStatusCodeString()); }
> --8<---------------cut here---------------end--------------->8---
>
> Joke aside :-), it would be *really* great if people could make an
> effort sticking to the convention of using no more than 120 characters
> per line which is already enough to make my eyes bleed.
>
> Thanks.
>

[Mathieu Lirzin]

Hello Jacques,

Jacques Le Roux <[hidden email]> writes:

> Yes you are right, the whole file should be reformatted.
>
> This "around 120 chars max" rule is "new" (few years) and most of the
> code there is more than a decade.

OK, sure.

> If nobody disagree we could have a task Jira to reformat the code of
> the most important classes (with subtasks maybe, or simply patches for
> concerned classes).

I sympathise but I am not sure about this strategy, which depending on
the capabilities of your VCS might obscure the commit history.  I would
recommend to simply use the “120 chars max” guideline for newly added
code and when refactoring existing one.

WDYT?

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Gil Portenseigne]

Hello,

+1 to follow the 120char guideline for any commit that the purpose it
not to refactor line length :).

Gil


Le vendredi 26 oct. 2018 à 11:13:48 (+0200), Mathieu Lirzin a écrit :

> Hello Jacques,
>
> Jacques Le Roux <[hidden email]> writes:
>
> > Yes you are right, the whole file should be reformatted.
> >
> > This "around 120 chars max" rule is "new" (few years) and most of the
> > code there is more than a decade.
>
> OK, sure.
>
> > If nobody disagree we could have a task Jira to reformat the code of
> > the most important classes (with subtasks maybe, or simply patches for
> > concerned classes).
>
> I sympathise but I am not sure about this strategy, which depending on
> the capabilities of your VCS might obscure the commit history.  I would
> recommend to simply use the “120 chars max” guideline for newly added
> code and when refactoring existing one.
>
> WDYT?
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Michael Brohl-3]

+1 for not reformatting all at once but only if there are changes anyway.

The patch should mention this because it will make reviews more complex.

Regards,

Michael


Am 26.10.18 um 11:13 schrieb Mathieu Lirzin:

> Hello Jacques,
>
> Jacques Le Roux <[hidden email]> writes:
>
>> Yes you are right, the whole file should be reformatted.
>>
>> This "around 120 chars max" rule is "new" (few years) and most of the
>> code there is more than a decade.
> OK, sure.
>
>> If nobody disagree we could have a task Jira to reformat the code of
>> the most important classes (with subtasks maybe, or simply patches for
>> concerned classes).
> I sympathise but I am not sure about this strategy, which depending on
> the capabilities of your VCS might obscure the commit history.  I would
> recommend to simply use the “120 chars max” guideline for newly added
> code and when refactoring existing one.
>
> WDYT?
>

[Jacques Le Roux]

Le 26/10/2018 à 11:13, Mathieu Lirzin a écrit :
> I sympathise but I am not sure about this strategy, which depending on
> the capabilities of your VCS might obscure the commit history.  I would
> recommend to simply use the “120 chars max” guideline for newly added
> code and when refactoring existing one.
>
> WDYT?
Yes, that's what we decided already and I'm all for it. It's indeed the only way if we don't want to blur things in svn history.

I simply forgot to format before committing

I just did it at r1844909

Jacques

[Mathieu Lirzin]

Jacques Le Roux <[hidden email]> writes:

> Le 26/10/2018 à 11:13, Mathieu Lirzin a écrit :
>> I sympathise but I am not sure about this strategy, which depending on
>> the capabilities of your VCS might obscure the commit history.  I would
>> recommend to simply use the “120 chars max” guideline for newly added
>> code and when refactoring existing one.
>>
>> WDYT?
> Yes, that's what we decided already and I'm all for it. It's indeed the only way if we don't want to blur things in svn history.
>
> I simply forgot to format before committing
>
> I just did it at r1844909

Thanks!

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[taher]

As I said countless times so far, mostly to Jacques, let's please try
to avoid the "I have an idea, now let's do it everywhere in the code
base" kind of approach. Slow gradual refactoring is the way to go to
ensure sustainable quality and stability.

Therefore, I'm in favor of Mathieu's approach.

On Fri, Oct 26, 2018 at 12:13 PM Mathieu Lirzin
<[hidden email]> wrote:

>
> Hello Jacques,
>
> Jacques Le Roux <[hidden email]> writes:
>
> > Yes you are right, the whole file should be reformatted.
> >
> > This "around 120 chars max" rule is "new" (few years) and most of the
> > code there is more than a decade.
>
> OK, sure.
>
> > If nobody disagree we could have a task Jira to reformat the code of
> > the most important classes (with subtasks maybe, or simply patches for
> > concerned classes).
>
> I sympathise but I am not sure about this strategy, which depending on
> the capabilities of your VCS might obscure the commit history.  I would
> recommend to simply use the “120 chars max” guideline for newly added
> code and when refactoring existing one.
>
> WDYT?
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37