Skip to content
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

ZonedDateTime Property #477

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

ZonedDateTime Property #477

wants to merge 7 commits into from

Conversation

chace86
Copy link
Collaborator

@chace86 chace86 commented Oct 31, 2019

WIP! Working on tests for property.

@chace86 chace86 marked this pull request as ready for review October 31, 2019 05:13
Copy link
Owner

@eeverman eeverman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this one one - Nice progress!

@@ -7,7 +7,7 @@
import org.yarnandtail.andhow.valuetype.LocalDateTimeType;

/**
* A Property that refers to a Long value.
* A Property that refers to a LocalDateTime value.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch - I wonder how long that has been there??

*
* By default this uses the TrimToNullTrimmer, which removes all whitespace from
* the value and ultimately null if the value is all whitespace. The String
* constructor version is used when creating instances of BigDecimal.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note about 'BigDecimal' can just be deleted. But, it would be nice to note how the string value is interpreted in this class. There is a note about string date format further down.

*
* @author chace86
*/
public class ZonedDateTimeProp extends PropertyBase<ZonedDateTime> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you started from BigDecimal as a template for this class, but LocalDateTime might be a better place to start. The builder methods in LocalDateTime use time related terms like before and after, rather that greater than or less than.

* @param reference value the property must be greater than
* @return the builder instance
*/
public ZonedDateTimeBuilder mustBeGreaterThan(ZonedDateTime reference) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See naming in LocalDateTime for all.

* Extended by nested static classes. The nested classes implement
* constraints that may be used when building the property.
*/
public abstract class ZonedDateTimeValidator implements Validator<ZonedDateTime> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar, refer to LocalDateTimeValidator.java for method names, ie before/after.

try {
return ZonedDateTime.parse(sourceValue);
} catch (Exception e) {
throw new ParsingException("Unable to convert to a LocalDateTime", sourceValue, e);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ZonedDateTime


if (sourceValue != null) {
try {
return ZonedDateTime.parse(sourceValue);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to make this easier to construct. The default parse method requires the square bracket construction where as most users don't care about the named region, just the hour offset. Can we switch to this format/parser: https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html#ISO_ZONED_DATE_TIME

That should allow either.

@eeverman eeverman changed the base branch from master to main July 22, 2021 21:24
@eeverman
Copy link
Owner

Hey @chace86 - Do you have any interest in picking this back up?
This project has come a long way in the last few months - I hope you might consider taking a look.

FYI - The naming conventions have changed a bit for the builder methods: The 'must' prefix has been removed. Compare to the LocalDateTimeProp (the non-deprecated methods)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants