-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JXLS 3.0.0 #284
JXLS 3.0.0 #284
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Less code to maintain is always better.
I assume we just need to fix the tests now.
Should we also set the Java 17 language level in pom.xml and in build.gradle?
- slf4j 2.0.9 - (test) surefire 3.2.2 - (test) junit.. 5.10.1 - (test) Mockito 4.11 (not 5.x) - (test) Spock 2.4-M1 (keep Groovy 3) - POI 5.2.5 - c.-compress 1.25 - (test) c.-io 2.15.1 - (test) not updated: Derby
@leonate I need your help for generating the site. See file .github/maven.yml |
- you must clean project in Eclipse before launching testcase - Driver class does not exist in newer classes and Driver isn't found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work!
Awesome work !!! Before I could raise hand to help , I can see its already done. |
Thanks. @vgaur We could need help migrating the Groovy testcases to Java. |
write("ERROR", msg, e); | ||
} | ||
|
||
protected void write(String level, String msg, Throwable e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new Jxls logging system is simple but seems to be very limited and I wonder if we are not reinventing the wheel here?
E.g. how one would go if they would like to have all the Jxls logging go to specific file, disable certain logging levels, have automatic file rotation depending on size etc. And if we want to support any of this, should we perhaps consider using the standard Java logging interface from java.util.logging
instead of a custom solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it's good to have something between the Jxls code and the logging framework. And that's JxlsLogger. In the past it was a problem to control the logging (vs. throwing exceptions).
A Jxls user can copy e.g. JxlsLoggerForSlf4j.java from our homepage to his workspace and add the slf4j dependencies.
We use PoiExceptionThrower (for standard cases) and someone catch those messages and will log (or display) them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I get the idea now. It's fine to have a middleman between a logger framework and Jxls. I just want us to ensure Jxls can be integrated into other apps that might be using a different logging system.
w.selectSheet("Employees"); | ||
assertEquals("Sascha", w.getCellValueAsString(4, 1)); | ||
assertEquals("direction=RIGHT does not work", "H", w.getCellValueAsString(4, 8)); | ||
assertFalse(w.getColumnWidth(4) == w.getColumnWidth(5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above you use assertEquals
and here assertFalse
for unequality. Would not it be better to use assertNotEquals(w.getColumnWidth(4), w.getColumnWidth(5));
for consistency instead of assertFalse
?
|
||
public EmployeeWithDepartments(String name, String birthDate, double payment, String ...departments) throws ParseException { | ||
super(name, dateFormat.parse(birthDate), payment, 0d); | ||
for (String i : departments) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can use this.departments.addAll(Arrays.asList(departments));
instead of the for
loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it work with the Java 17 Record type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
This reverts commit 8805c8f.
|
pom.xml files have to be changed
@leonate tell me when I can merge |
|
Planned: