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

Implement support for LocalDate and OffsetDateTime in RequestInformation #1586

Merged
merged 11 commits into from
Oct 3, 2024
Merged

Implement support for LocalDate and OffsetDateTime in RequestInformation #1586

merged 11 commits into from
Oct 3, 2024

Conversation

espenvis
Copy link
Contributor

@espenvis espenvis commented Sep 27, 2024 •
edited
Loading

Implements behavior for LocalDate and OffsetDateTime in RequestInformation's getSanitizedValues. Added accompanying test for LocalDate path parameter resolution.

Edit: scope is extended to include LocalTime and PeriodAndDuration.

closes #1583

@espenvis
add condition for value instanceof OffsetDateTime and LocalDate in Re…
6097b56 
…questInformation#getSanitizedValues, add test for LocalDate pathParameter resolution
@espenvis espenvis requested a review from a team as a code owner September 27, 2024 14:06
@espenvis espenvis marked this pull request as draft September 27, 2024 16:42
@espenvis
Copy link
Contributor Author

Marking as draft until we can evaluate extending this issue to tackle time and duration amongst other types.

baywet
baywet requested changes Oct 2, 2024
View reviewed changes
Copy link
Member

@baywet baywet 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 the contribution!
Could you also bump the dependency now that it's been released please?
(this way we can check there are no regressions)

components/abstractions/src/main/java/com/microsoft/kiota/RequestInformation.java Show resolved Hide resolved
@espenvis
Copy link
Contributor Author

espenvis commented Oct 3, 2024

Updated to include time and duration datatype support (LocalTime and PeriodAndDuration). Also merged upstream changes. The version is now bumped up to 1.5.0 and all tests pass. Let me know if I've missed something.

@espenvis espenvis requested a review from baywet October 3, 2024 11:04
@espenvis espenvis marked this pull request as ready for review October 3, 2024 11:05
baywet
baywet reviewed Oct 3, 2024
View reviewed changes
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!
Can you also bump std uri template to v2 so we can confirm there's no end to end regression please?

kiota-java/components/abstractions/gradle/dependencies.gradle

Line 11 in bf66598

implementation 'io.github.std-uritemplate:std-uritemplate:1.0.6'

baywet
baywet previously approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!

@espenvis
Copy link
Contributor Author

espenvis commented Oct 3, 2024 •
edited
Loading

Thank you for making the changes! Can you also bump std uri template to v2 so we can confirm there's no end to end regression please?

kiota-java/components/abstractions/gradle/dependencies.gradle

Line 11 in bf66598

implementation 'io.github.std-uritemplate:std-uritemplate:1.0.6'

Yes. Apologies, I missed your earlier question.

It seems this causes an exception in UriTemplateTest.java:32 - this passes parameters directly to StdUriTemplate and will fail.

Edit: is this test appropriate? I imagine we would run the parameters through getSanitizedValues first.

@baywet baywet enabled auto-merge (squash) October 3, 2024 11:16
@baywet baywet disabled auto-merge October 3, 2024 11:16
@baywet
Copy link
Member

baywet commented Oct 3, 2024

@espenvis thanks for the information.
I think you can remove the last line in the CSV and the date parameter in the unit test. (make sure you pull beforehand since I've applied formatting to the csv file)

@espenvis
Merge remote-tracking branch 'origin/fix/getsanitizedvalues-add-local…
0f3f3ff 
…date-and-offsetdatetime' into fix/getsanitizedvalues-add-localdate-and-offsetdatetime
@espenvis
Copy link
Contributor Author

espenvis commented Oct 3, 2024

@espenvis thanks for the information. I think you can remove the last line in the CSV and the date parameter in the unit test. (make sure you pull beforehand since I've applied formatting to the csv file)

Yep, that's what I figured. I'll do that.

@espenvis
fix: remove OffsetDateTime test in UriTemplateTest and accompanying t…
831097d 
…emplate in data.csv resource, no longer applicable in std-uritemplate 2
@espenvis
Copy link
Contributor Author

espenvis commented Oct 3, 2024

Test still fails due to a JUnit error of some sort. I'll have a look.

@espenvis
Copy link
Contributor Author

espenvis commented Oct 3, 2024

Seems it complained about the change in delimiter. Pushing a fix for that.

@espenvis espenvis requested a review from baywet October 3, 2024 11:44
baywet
baywet previously approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!

@baywet baywet enabled auto-merge (squash) October 3, 2024 11:51
@baywet baywet merged commit df6208a into microsoft:main Oct 3, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baywet baywet baywet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Kiota
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

prepare for std uri template 2 and date time drop
2 participants
@espenvis @baywet