Add limits overriding with system properties #112
Conversation
|
Thanks for contributing to FastCSV! Please add tests. |
| System.err.println("Invalid format for system property " + key); | ||
| throw e; |
There was a problem hiding this comment.
No console logging allowed. Just re-throw with more context information.
| @@ -27,4 +34,24 @@ public final class Limits { | |||
| private Limits() { | |||
| } | |||
|
|
|||
| /** | |||
| * Retrieves the system property value as an integer, with a fallback to a default value. | |||
| * If the property is not set or cannot be parsed, the default value is returned. | |||
There was a problem hiding this comment.
The implementation does not use the default value if it cannot be parsed (which is good IMO). The same applies to the documentation of the defaultValue and return value.
| */ | ||
| private static int getIntProperty(String key, int defaultValue) { | ||
| String value = System.getProperty(key); | ||
| if (value != null) { |
There was a problem hiding this comment.
Please invert the logic to have shorter branch.
| @@ -2,6 +2,13 @@ | |||
|
|
|||
| /** | |||
| * The {@code Limits} class defines the maximum limits for various fields and records in a CSV file. | |||
| * <p> | |||
| * Properties can be overridden by using | |||
| return Integer.parseInt(value); | ||
| } catch (NumberFormatException e) { | ||
| System.err.println("Invalid format for system property " + key); | ||
| throw e; |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #112 +/- ##
============================================
+ Coverage 98.32% 98.34% +0.01%
- Complexity 390 393 +3
============================================
Files 31 32 +1
Lines 1136 1145 +9
Branches 160 161 +1
============================================
+ Hits 1117 1126 +9
Misses 9 9
Partials 10 10 ☔ View full report in Codecov by Sentry. |
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; |
There was a problem hiding this comment.
FastCSV uses AssertJ-Assertions exclusively.
| @BeforeEach | ||
| void setup() { | ||
| System.clearProperty("fastcsv.max.field.size"); | ||
| System.clearProperty("fastcsv.max.field.count"); | ||
| } |
There was a problem hiding this comment.
I don't understand why this is required. Isn't AfterEach sufficient?
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class LimitsTest { |
There was a problem hiding this comment.
The two properties (fastcsv.max.field.size and fastcsv.max.field.count) should be declared as constants in order to not repeat them over and over again...
|
Thank you! |
This PR relates to Issue: #104
Proposed Changes