Closed wardt7 closed 2 years ago
Have made a few small tweaks (mostly content+comments), but had to make them on a native branch so closing this in favour of https://github.com/alphagov/notifications-java-client/pull/211. Will continue the conversation there.
What problem does the pull request solve?
prepareUpload
uses aString
to represent the duration forretentionPeriod
retentionPeriod
, you have to start performing string manipulation to get there, which is easy to get wrong. This problem could potentially get worse if you start to introduce other units of time aside from weeksChronoUnit
are available to users, with aString
being a relatively loose data type. If a developer were to write some code using prepareUpload, it would be easy for them to accidentally pass in a different string without realising, or for the String being passed in before hand to be manipulated.What is the Solution?
RetentionPeriodDuration
, strictly takes an amount as anInteger
, and a unit of time as aChronoUnit
.prepareUpload
ChronoUnit
(e.g. Months / Years) if the days unit were to be added to the API in the future. This is because you could modify the constructor to convert the amount to days depending on theChronoUnit
(e.g. WEEKS would multiply the amount by 7).Other Notes
prepareUpload
instead of replacing it (and for the most part uses the original function), so existing users of the function won't be affected and maintenance should be fairly simpleChecklist
DOCUMENTATION.md
)src/main/resources/application.properties
pom.xml