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

Improve validation for Rule 240 #1245

Merged
merged 1 commit into from
Jun 3, 2021
Merged

Improve validation for Rule 240 #1245

merged 1 commit into from
Jun 3, 2021

Conversation

vadeg
Copy link
Contributor

@vadeg vadeg commented May 29, 2021

  • Simplify code
  • Add a fix when boolean enums are validated
  • Add a type check for enum value types and a property type
  • Fix variable shadowing problem
  • Make extensibleEnum extension return Any instead of String

Related to #1221 and #1183

* Simplify code
* Add a fix when `boolean` enums are validated
* Add a type check for enum value types and a property type
* Fix variable shadowing problem
* Make `extensibleEnum` extension return `Any` instead of `String`
@vadeg
Copy link
Contributor Author

vadeg commented May 29, 2021

@tkrop this PR fixes the problem with the Rule 240 you've mentioned

@codecov-commenter
Copy link

Codecov Report

Merging #1245 (5cc6bc8) into master (90b0ddd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1245   +/-   ##
=========================================
  Coverage     87.62%   87.62%           
  Complexity       85       85           
=========================================
  Files            69       69           
  Lines           978      978           
  Branches         71       71           
=========================================
  Hits            857      857           
  Misses           84       84           
  Partials         37       37           

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 90b0ddd...5cc6bc8. Read the comment docs.

Copy link
Member

@tkrop tkrop left a comment

Choose a reason for hiding this comment

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

@vadeg sorry, for the review delay. I have some questions left.

validatePrimitiveSchemas(context) + validateAllProperties(context)

private fun validateAllProperties(context: Context): List<Violation> = context.api.getAllProperties()
.filter { it.value.type == "string" }
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we need to check independent of property type for value definitions? Currently, it looks as if they are just not checked. However, I must confess, I have no idea how numbers 1, 2, and true, false would behave in this context.

@tkrop
Copy link
Member

tkrop commented Jun 1, 2021

👍

@vadeg
Copy link
Contributor Author

vadeg commented Jun 3, 2021

👍

@vadeg vadeg merged commit c389e8a into master Jun 3, 2021
@tkrop tkrop deleted the fix-rule-240 branch June 3, 2021 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants