Re: svn commit: r1864832 - in /ofbiz/ofbiz-framework/trunk: applications/accounting/config/ applications/order/template/order/ applications/order/widget/ordermgr/ framework/base/src/main/java/org/apac
Nicolas Malin writes:
> On 8/10/19 12:07 AM, Mathieu Lirzin wrote:
>
>> Here are a few inline comments regarding the code.
>>
>> Maybe I overlooked some good reason justifying some design decision you
>> made, so if you want to discuss more about the suggestions I proposed,
>> we can do some pair programming next week.
>
> It was a big task, so I focused my mind only on how homogenize and
> centralize a number displaying. So if you I have some idea to continue
> the improvement through other design concept, It's welcome :)
This is indeed a big effort which is adding a great amount of
flexibility.
>>>*/
>>> -public static String formatPrice(Double price) {
>>> -if (price == null) {
>>> +public static String formatNumber(Double number, String formatType,
>>> Delegator delegator, Locale locale) {
>> The dependency on the Delegator should be avoided if possible because it
>> makes writing unit complex tests. By the way where are those tests ? :-)
> Normally not because delegator and locale are optional. If it's not
> the case, I will improve it to support unit test
To me this is the only critical part that needs to be fixed because
previous implementation was achieving better testability. Other remarks
are less important improvements that can be worked on later.
>> To remove a dependency on a class which is hard to instantiate like
>> ‘Delegator’, the common solution is to replace it with an interface
>> (ideally a functional interface to let the caller pass a lambda).
>>
>> In this particular case after a quick look, the ‘delegator’ argument
>> could potentially be replaced by a ‘templateProvider’ argument of type
>> ‘Function’ where the input is a format type and the
>> output is a template.
>>
>> When things are complicated to decouple, an alternative is write an
>> overload specifically for the test and use it inside the coupled one.
> I's seem to be a good idea :) but I'm far away to understand what do
> you explain, I need to studies more ^^
Decoupling methods from stateful objects like ‘Delegator’,
‘HttpServletrequest’ and ‘GenericValue’ is hard to explain and
understand theorically, however in practice when adopting the hygiene of
writing unit tests with plain java objects it become second nature. :-)
Thanks.
--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
Re: svn commit: r1864881 - /ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
Jacques Le Roux writes: > Done Thanks Jacques. -- Mathieu Lirzin GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
Re: [VOTE] [RELEASE] Apache OFBiz 16.11.06
Thanks Jacopo -1 checksum OK, tests OK, runs OK Before being able to run tests and OFBiz, like you Jacopo, I had to add org.gradle.jvmargs="-Xmx512m" in a gradle.properties file (for me in Gradle home) I wonder if it's not related to the Gradle version installed. After seeing that Swapnil was able to run with 5.5.1 I tried it, to no avail, same again back to 5.0. Only adding the trick in a gradle.properties file worked. Maybe we should better have DEFAULT_JVM_OPTS="" in both Gradle files? Did not try... Also the README.md.html was not up to date. I committed the necessary change. We could also get rid of it, but I find it quite convenient :) Jacques Le 11/08/2019 à 14:07, Nicolas Malin a écrit : Same here +1 On 8/10/19 4:30 PM, Taher Alkhateeb wrote: All tests are clear, smoke tests clear, SHA512 checksum matches +1 from my side. Thank you for your efforts Jacopo On Sat, Aug 10, 2019 at 12:17 PM Jacopo Cappellato wrote: This is the vote thread to release a new bug fix release for the release16.11 branch. This new release, "Apache OFBiz 16.11.06" will supersede all the previous releases from the same branch. The release files can be downloaded from here: https://dist.apache.org/repos/dist/dev/ofbiz/ and are: * apache-ofbiz-16.11.06.zip * KEYS: text file with keys * apache-ofbiz-16.11.06.zip.asc: the detached signature file * apache-ofbiz-16.11.06.zip.sha512: checksum file Please download and test the zip file and its signatures (for instructions on testing the signatures see http://www.apache.org/info/verification.html). The preview of the Release Notes can be found at: https://ofbiz.apache.org/release-notes-16.11.06.html Vote: [ +1] release as Apache OFBiz 16.11.06 [ -1] do not release In order to provide enough time to discuss and test the candidate files, this vote will be open for at least 10 days. For more details about this process please read http://www.apache.org/foundation/voting.html Best Regards, Jacopo Cappellato
Re: buildbot failure in on ofbizBranch17Framework
Actually no error there either: https://ci.apache.org/projects/ofbiz/logs/17.12/framework/html/ Le 11/08/2019 à 15:34, build...@apache.org a écrit : The Buildbot has detected a new failure on builder ofbizBranch17Framework while building . Full details are available at: https://ci.apache.org/builders/ofbizBranch17Framework/builds/322 Buildbot URL: https://ci.apache.org/ Buildslave for this Build: asf947_ubuntu Build Reason: The AnyBranchScheduler scheduler named 'onBranch17FrameworkCommit' triggered this build Build Source Stamp: [branch ofbiz/ofbiz-framework/branches/release17.12] 1864927 Blamelist: jleroux BUILD FAILED: failed shell_2 Sincerely, -The Buildbot
Re: svn commit: r1864881 - /ofbiz/ofbiz-framework/trunk/applications/product/src/main/java/org/apache/ofbiz/product/imagemanagement/FrameImage.java
Done Le 10/08/2019 à 19:29, Jacques Le Roux a écrit : Makes sense Mathieu, I'll... Jacques Le 10/08/2019 à 18:43, Mathieu Lirzin a écrit : Hello Jacques, jler...@apache.org writes: - File dir = new File(imageServerPath + dirPath); + File dir = new File(imageServerPath + dirPath).toPath().normalize().toFile(); // cf. OFBIZ-9973 IMO This comment should include a short rationale too. Referencing a bug report is really useful when the issue is complex and the reader might want to understand the details leading to a non-obvious piece of code, but it would be better to *not* require the developers to do the extra work of looking at JIRA to get a broad idea of the rationale. I would recommend the something like: --8<---cut here---start->8--- // Normalize the path because we want to ensure that ... (see OFBIZ-9973 for more details). --8<---cut here---end--->8--- Additionally in order to avoid repeating such comment in multiple places creating a helper method factorizing the code related to fixing that issue and putting the comment ontop of the method definition is nice too. My 2¢. Thanks Jacques.
Re: svn commit: r1864832 - in /ofbiz/ofbiz-framework/trunk: applications/accounting/config/ applications/order/template/order/ applications/order/widget/ordermgr/ framework/base/src/main/java/org/apac
On 8/10/19 12:07 AM, Mathieu Lirzin wrote:
Hello Nicolas,
Hi man,
Here are a few inline comments regarding the code.
Maybe I overlooked some good reason justifying some design decision you
made, so if you want to discuss more about the suggestions I proposed,
we can do some pair programming next week.
It was a big task, so I focused my mind only on how homogenize and
centralize a number displaying. So if you I have some idea to continue
the improvement through other design concept, It's welcome :)
[...]
@@ -34,16 +34,11 @@ import java.util.TimeZone;
public final class UtilFormatOut {
public static final String module = UtilFormatOut.class.getName();
-
-// --- price format handlers ---
-// FIXME: This is not thread-safe! DecimalFormat is not synchronized.
-private static final DecimalFormat priceDecimalFormat = new
DecimalFormat(UtilProperties.getPropertyValue("general", "currency.decimal.format",
"#,##0.00"));
-
-// --- quantity format handlers ---
-private static final DecimalFormat quantityDecimalFormat = new
DecimalFormat("#,##0.###");
-
-// --- percentage format handlers ---
-private static final DecimalFormat percentageDecimalFormat = new
DecimalFormat("##0.##%");
+public static final String DEFAULT_FORMAT = "default";
+public static final String AMOUNT_FORMAT = "amount";
+public static final String QUANTITY_FORMAT = "quantity";
+public static final String PERCENTAGE_FORMAT = "percentage";
+public static final String SPELLED_OUT_FORMAT = "spelled-out";
Intuitively an ‘enum’ would seem more appropriate to make the relation
between those constants clearer.
--8<---cut here---start->8---
enum Format {
DEFAULT, AMOUNT, QUANTITY, PERCENTAGE, SPELLED_OUT;
// Converts a string to a typed value.
static Format valueOf(String name) {
...
}
}
--8<---cut here---end--->8---
Additionally I would be nice if the meaning of those “symbols” could be
documented inside the code either in the XSD with a reference in the
Java code, or in both in XSD and Java.
Indeed
private UtilFormatOut() {}
@@ -54,43 +49,70 @@ public final class UtilFormatOut {
return "";
}
-/** Formats a Double representing a price into a string
- * @param price The price Double to be formatted
- * @return A String with the formatted price
+/** Format a number with format type define by properties
+ *
Please expand the javadoc instead of removing it.
I will check, maybe it's an error
*/
-public static String formatPrice(Double price) {
-if (price == null) {
+public static String formatNumber(Double number, String formatType,
Delegator delegator, Locale locale) {
The dependency on the Delegator should be avoided if possible because it
makes writing unit complex tests. By the way where are those tests ? :-)
Normally not because delegator and locale are optional. If it's not the
case, I will improve it to support unit test
To remove a dependency on a class which is hard to instantiate like
‘Delegator’, the common solution is to replace it with an interface
(ideally a functional interface to let the caller pass a lambda).
In this particular case after a quick look, the ‘delegator’ argument
could potentially be replaced by a ‘templateProvider’ argument of type
‘Function’ where the input is a format type and the
output is a template.
When things are complicated to decouple, an alternative is write an
overload specifically for the test and use it inside the coupled one.
I's seem to be a good idea :) but I'm far away to understand what do you
explain, I need to studies more ^^
+if (number == null) {
return "";
}
-return formatPrice(price.doubleValue());
+if (formatType == null) {
+formatType = DEFAULT_FORMAT;
+}
+if (locale == null) {
+locale = Locale.getDefault();
+}
Does silencing ‘null’ values really make sense here? if ‘null’ values
doesn't have sound meaning, I would recommend bailing out early instead
of silencing a possible programming mistake or worse let another method
downstream throw a cryptic exception later. To bail out early you can
use things like ‘Objects.requireNonNull(myArg)’.
I use the silencing because we are on displaying number so instead
deploy complex analyze with risk to break something during the
rendering, I prefer to use safe call en just return empty String.
Have a nice weekend!
Thanks for your time and remarks
Nicolas
Re: [VOTE] [RELEASE] Apache OFBiz 16.11.06
Same here +1 On 8/10/19 4:30 PM, Taher Alkhateeb wrote: All tests are clear, smoke tests clear, SHA512 checksum matches +1 from my side. Thank you for your efforts Jacopo On Sat, Aug 10, 2019 at 12:17 PM Jacopo Cappellato wrote: This is the vote thread to release a new bug fix release for the release16.11 branch. This new release, "Apache OFBiz 16.11.06" will supersede all the previous releases from the same branch. The release files can be downloaded from here: https://dist.apache.org/repos/dist/dev/ofbiz/ and are: * apache-ofbiz-16.11.06.zip * KEYS: text file with keys * apache-ofbiz-16.11.06.zip.asc: the detached signature file * apache-ofbiz-16.11.06.zip.sha512: checksum file Please download and test the zip file and its signatures (for instructions on testing the signatures see http://www.apache.org/info/verification.html). The preview of the Release Notes can be found at: https://ofbiz.apache.org/release-notes-16.11.06.html Vote: [ +1] release as Apache OFBiz 16.11.06 [ -1] do not release In order to provide enough time to discuss and test the candidate files, this vote will be open for at least 10 days. For more details about this process please read http://www.apache.org/foundation/voting.html Best Regards, Jacopo Cappellato
Re: buildbot failure in on ofbizBranch17FrameworkPlugins
OK, locally Le 10/08/2019 à 20:29, build...@apache.org a écrit : The Buildbot has detected a new failure on builder ofbizBranch17FrameworkPlugins while building . Full details are available at: https://ci.apache.org/builders/ofbizBranch17FrameworkPlugins/builds/362 Buildbot URL: https://ci.apache.org/ Buildslave for this Build: asf947_ubuntu Build Reason: downstream Build Source Stamp: [branch ofbiz/ofbiz-framework/branches/release17.12] 1864893 Blamelist: jleroux BUILD FAILED: failed shell_5 Sincerely, -The Buildbot
