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

Implement struct valued constant validator for loading schemas #467

Merged
merged 2 commits into from
Apr 13, 2022

Conversation

janarajan
Copy link
Contributor

This commit resolves #466

@ghost
Copy link

ghost commented Apr 11, 2022

CLA assistant check
All CLA requirements met.

@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2022

Codecov Report

Merging #467 (8d8df1d) into master (c8751a2) will increase coverage by 0.08%.
The diff coverage is 68.75%.

@@             Coverage Diff              @@
##             master     #467      +/-   ##
============================================
+ Coverage     58.79%   58.87%   +0.08%     
+ Complexity      866      861       -5     
============================================
  Files            69       69              
  Lines          6436     6451      +15     
  Branches       1002     1006       +4     
============================================
+ Hits           3784     3798      +14     
+ Misses         2362     2360       -2     
- Partials        290      293       +3     
Impacted Files Coverage Δ
...in/kotlin/com/microsoft/thrifty/schema/Constant.kt 78.53% <68.75%> (+0.13%) ⬆️
.../com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt 84.10% <0.00%> (ø)
...main/kotlin/com/microsoft/thrifty/schema/Linker.kt 91.41% <0.00%> (+1.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8751a2...8d8df1d. Read the comment docs.

Copy link
Collaborator

@benjamin-bader benjamin-bader left a comment

Choose a reason for hiding this comment

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

Nice work! A few small nits but this is basically good-to-go.

// this should work for exception type as well. structType has isException field
return STRUCT
}

throw IllegalStateException("Struct-valued constants are not yet implemented")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need a new message for this exception :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) Yup, fixed the messaging

Comment on lines 401 to 412
var keyMatchedField: Field? = null
val keyString = key.value
for (field in fields) {
if (field.name == keyString) {
keyMatchedField = field
break
}
}
if (keyMatchedField == null) {
throw IllegalStateException("${expected.name} struct has no field ${key.value}")
}
Constant.validate(symbolTable, value, keyMatchedField.type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var keyMatchedField: Field? = null
val keyString = key.value
for (field in fields) {
if (field.name == keyString) {
keyMatchedField = field
break
}
}
if (keyMatchedField == null) {
throw IllegalStateException("${expected.name} struct has no field ${key.value}")
}
Constant.validate(symbolTable, value, keyMatchedField.type)
val field = fields.firstOrNull { it.name == key.value } ?: error("${expected.name} struct has no field ${key.value}")
Constant.validate(symbolTable, value, field.type)


const Region DEFAULT_REGION = {
1 : "US",
"isActive" : true // field name does not exist in Region
Copy link
Collaborator

Choose a reason for hiding this comment

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

isActive does in fact exist; should the comment be for the line above this one?

@benjamin-bader
Copy link
Collaborator

Thanks! Looks good; assuming tests pass, please sign the CLA and we'll be good to merge.

// this should work for exception type as well. structType has isException field
return STRUCT
}

throw IllegalStateException("Struct-valued constants are not yet implemented")

Choose a reason for hiding this comment

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

Change the exception message since Struct-valued constants are now supported.

Choose a reason for hiding this comment

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

nevermind... the message was updated in the latest update!

@janarajan
Copy link
Contributor Author

@benjamin-bader thanks for the review and approval. I have now signed the CLA so you can accept and merge the PR. Let me know.

@benjamin-bader benjamin-bader merged commit f534893 into microsoft:master Apr 13, 2022
@janarajan
Copy link
Contributor Author

@benjamin-bader thanks for merging the PR. What is the release cycle? Any possibility of releasing a candidate with this change?

@benjamin-bader
Copy link
Collaborator

There's no official schedule; releases happen whenever there's enough to justify a release. I'll see about making an RC soon but am not confident I have my laptop properly configured for it right now - it might take a little while.

In the meantime I believe you can use a snapshot build.

@benjamin-bader
Copy link
Collaborator

Version 3.1.0-RC01 should be up in Maven Central in the next 10 minutes or so

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.

Struct-valued constants are not yet implemented
5 participants