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

Adds readers for element and logic constraints #260

Merged
merged 1 commit into from
May 19, 2023

Conversation

popematt
Copy link
Contributor

@popematt popematt commented May 17, 2023

Issue #, if available:

#256

Description of changes:

Implements readTypeArg() and adds readers for some constraints that depend on type arguments (element and the "logic" constraints). Updates ReaderTests to cover all of the constraints readers that were added and the $null_or test cases.

Related PRs in ion-schema, ion-schema-tests, ion-schema-schemas:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

import org.junit.jupiter.params.provider.ValueSource

@ExperimentalIonSchemaModel
class LogicConstraintsReaderTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ The tests here are intentionally sparse. canRead is completely covered, but tests for read cover only a few cases since comprehensive testing is handled with the ion-schema-tests test cases.

@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

❗ No coverage uploaded for pull request base (master@1827814). Click here to learn what that means.
Patch coverage: 57.14% of modified lines in pull request are covered.

❗ Current head c5d1c97 differs from pull request most recent head 84eb878. Consider uploading reports for the commit 84eb878 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #260   +/-   ##
=========================================
  Coverage          ?   82.72%           
  Complexity        ?      810           
=========================================
  Files             ?      118           
  Lines             ?     3057           
  Branches          ?      602           
=========================================
  Hits              ?     2529           
  Misses            ?      366           
  Partials          ?      162           
Impacted Files Coverage Δ
...amazon/ionschema/reader/internal/TypeReaderV2_0.kt 42.85% <36.36%> (ø)
...der/internal/constraints/LogicConstraintsReader.kt 63.63% <63.63%> (ø)
...ema/reader/internal/constraints/ElementV2Reader.kt 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@popematt
Copy link
Contributor Author

popematt commented May 17, 2023

I'm not sure why Codecov says that some of the lines aren't covered, but it's possibly related to #221.

Edit—switching to kover reports code coverage that aligns with what I expected it to be. I'll make the change to kover in a follow-up PR.

@popematt popematt requested review from tgregg and desaikd May 17, 2023 23:11
}
ion is IonStruct -> {
islRequire(!ion.containsKey("name")) { "Inline type definitions may not have a 'name' field" }
islRequire(!ion.containsKey("occurs")) { "Inline type definitions may not have a 'occurs' field" }
Copy link
Contributor

Choose a reason for hiding this comment

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

(non-blocking) If this will be used by group of constraints then we should probably add a check here that allows occurs for ordered_elements and fields. If you are planning to do this in a separate PR then you can disregard this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's going to be handled by readVariablyOccurringTypeArg function (currently has a stub implementation on L70 of this file).

@popematt popematt merged commit 3a3f696 into amazon-ion:master May 19, 2023
3 checks passed
@popematt popematt deleted the read-impl-type branch June 13, 2024 22:32
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.

None yet

2 participants