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.

Sign up for GitHub

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

Jump to bottom

#482 accept date as string for chronozoneddatetime #625

Merged
merged 17 commits into from
Oct 15, 2020
Merged

#482 accept date as string for chronozoneddatetime #625

merged 17 commits into from
Oct 15, 2020

Conversation

Valefant
Copy link
Collaborator

@Valefant Valefant commented Oct 11, 2020 •
edited
Loading

Let's go

I confirm that I have read the Contributor Agreements v1.0, agree to be bound on them and confirm that my contribution is compliant.

@Valefant
Copy link
Collaborator Author

@robstoll I have one issues that I can't explain myself.

In DefaultChronoZonedDateTimeAssertions.kt lines 95-97 are showing as errors.
I tested the parseZonedDateTime in separate scratch File and it works without problems.

image

@robstoll
Copy link
Owner

Hard to guess without checking out your code. Will do that later on. Note that Intellij has a few bugs when it comes to JVM only. Does gradle output the same error? Are the functions you use all available in jdk8?

@Valefant
Copy link
Collaborator Author

Valefant commented Oct 13, 2020 •
edited
Loading

Hard to guess without checking out your code. Will do that later on. Note that Intellij has a few bugs when it comes to JVM only. Does gradle output the same error? Are the functions you use all available in jdk8?

Gradle does output the same error. I checked the functions, they are all available in jdk8.
I am using the jdk8 locally too. The code snippet works when I run it in a scratch file 🤔

Maybe I am missing something

@robstoll
Copy link
Owner

Maybe the problem is that you used Kotlin 1.4 in your worksheet whereas we still use Kotlin 1.2 in Atrium. Kotlin 1.4 improved the support for Java's SAM-interface interoperability. You can solve it as follows:

        val parsed = formatter.parseBest(
            data,
            TemporalQuery { temporal -> ZonedDateTime.from(temporal) },
            TemporalQuery { temporal -> LocalDateTime.from(temporal) },
            TemporalQuery { temporal -> LocalDate.from(temporal) }
        )

@Valefant
Copy link
Collaborator Author

@robstoll That was it, thank you 😄

Now some tests are failing because the DateTimeFormatter does not recognize a date like
2019-12-24T10:15:30.00000021+01:00[Europe/Zurich]. It is not containg the Z and additionally the text in the brackets [Europe/Zurich] is not parsed.

How should I handle this case?

@Valefant
Copy link
Collaborator Author

@robstoll That was it, thank you 😄

Now some tests are failing because the DateTimeFormatter does not recognize a date like
2019-12-24T10:15:30.00000021+01:00[Europe/Zurich]. It is not containg the Z and additionally the text in the brackets [Europe/Zurich] is not parsed.

How should I handle this case?

Ok I tried to implement a solution.
I am curious if it is any good

robstoll
robstoll requested changes Oct 13, 2020
View reviewed changes
...gic-jvm/src/main/kotlin/ch/tutteli/atrium/logic/impl/DefaultChronoZonedDateTimeAssertions.kt Outdated Show resolved Hide resolved
...gic-jvm/src/main/kotlin/ch/tutteli/atrium/logic/impl/DefaultChronoZonedDateTimeAssertions.kt Outdated Show resolved Hide resolved
...gic-jvm/src/main/kotlin/ch/tutteli/atrium/logic/impl/DefaultChronoZonedDateTimeAssertions.kt Outdated Show resolved Hide resolved
...en_GB-jvm/src/main/kotlin/ch/tutteli/atrium/api/infix/en_GB/chronoZonedDateTimeAssertions.kt Outdated
/**
* Expects that the subject of the assertion (a [ChronoZonedDateTime])
* is before or equals the [expected] [ChronoZonedDateTime] given as [String].
* Check [ch.tutteli.atrium.logic.impl.DefaultChronoZonedDateTimeAssertions.parseZonedDateTime] to see which values are allowed for [expected].
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 should write the supported formats here. I doubt someone wants to go and have a look at an internal method.
Also, we can then write that ZoneDesignator like [Europe/Zurich] is only supported experimentally.

Copy link
Owner

Choose a reason for hiding this comment

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

Nice work in api-fluent, can you please copy it to api-infix as well

.../main/kotlin/ch/tutteli/atrium/specs/integration/ChronoZonedDateTimeAsStringAssertionSpec.kt Outdated Show resolved Hide resolved
@Valefant Valefant changed the title #482 accept date as string for chronozoneddatetime (WIP) #482 accept date as string for chronozoneddatetime Oct 14, 2020
robstoll
robstoll requested changes Oct 14, 2020
View reviewed changes
Copy link
Owner

@robstoll robstoll left a comment

Choose a reason for hiding this comment

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

One last detail and we are good to merge

...en_GB-jvm/src/main/kotlin/ch/tutteli/atrium/api/infix/en_GB/chronoZonedDateTimeAssertions.kt Outdated
/**
* Expects that the subject of the assertion (a [ChronoZonedDateTime])
* is before or equals the [expected] [ChronoZonedDateTime] given as [String].
* Check [ch.tutteli.atrium.logic.impl.DefaultChronoZonedDateTimeAssertions.parseZonedDateTime] to see which values are allowed for [expected].
Copy link
Owner

Choose a reason for hiding this comment

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

Nice work in api-fluent, can you please copy it to api-infix as well

@robstoll robstoll merged commit 1a10575 into robstoll:master Oct 15, 2020
@robstoll robstoll linked an issue Oct 15, 2020 that may be closed by this pull request
accept date and time as string in ISO 8601 format for ChronoZonedDateTime #482
Closed
12 tasks
jakubriegel pushed a commit to jakubriegel/atrium that referenced this pull request Oct 16, 2020
# This is a combination of 2 commits.
a070825 
# This is the 1st commit message:

robstoll#630 add Path.isRelative method

# This is the commit message robstoll#2:

robstoll#482 accept date as string for chronozoneddatetime (robstoll#625)
jakubriegel pushed a commit to jakubriegel/atrium that referenced this pull request Oct 16, 2020
# This is a combination of 2 commits.
5e143e6 
# This is the 1st commit message:

robstoll#630 add Path.isRelative method

# This is the commit message robstoll#2:

robstoll#482 accept date as string for chronozoneddatetime (robstoll#625)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robstoll robstoll robstoll requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

accept date and time as string in ISO 8601 format for ChronoZonedDateTime
2 participants
@Valefant @robstoll