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

MODELIX-971 allProperties missing in ModelQl on INode #895

Closed
wants to merge 1 commit into from

Conversation

mhuster23
Copy link
Contributor

To be verified by reviewers

  • Relevant public API members have been documented
  • Documentation related to this PR is complete
    • Boundary conditions are documented
    • Exceptions are documented
    • Nullability is documented if used
  • Touched existing code has been extended with documentation if missing
  • Code is readable
  • New features and fixed bugs are covered by tests

Copy link
Contributor

Test Results

155 files  155 suites   11m 18s ⏱️
900 tests 891 ✅ 9 💤 0 ❌
910 runs  901 ✅ 9 💤 0 ❌

Results for commit 9728470.

Copy link
Contributor

JVM coverage report

Overall Project 53.11% -0.01%
Files changed 87.9% 🍏

File Coverage
AllPropertiesTraversalStep.kt 89.33% -10.67% 🍏
IProperty.kt 73.17% -12.2%
UntypedModelQL.kt 62.76% 🍏
INode.kt 29.73% -0.12%

@mhuster23 mhuster23 marked this pull request as ready for review July 11, 2024 11:25
@mhuster23 mhuster23 requested a review from slisson as a code owner July 11, 2024 11:25
}

override fun getOutputSerializer(serializationContext: SerializationContext): KSerializer<out IStepOutput<Pair<IProperty, String?>>> {
return serializationContext.serializer<Pair<IProperty, String?>>().stepOutputSerializer(this)
Copy link
Member

Choose a reason for hiding this comment

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

This only works, because the test case uses a CLTree with named based roles. Not all implementations of IProperty are serializable. I think we need https://issues.modelix.org/issue/MODELIX-976/Cleanup-relation-between-model-and-meta-model first and then return an IPropertyReference (or IPropertyId, not sure yet) instead, that can be resolved on the client side to an IPropertyDefinition if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it with a CLTree that uses useRoleIds = true and it works for that case as well.
But your proposed solution would definitely be cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

Because the role is always wrapped by a PropertyFromName, even if it is an ID. This really needs to be cleaned up.

@mhuster23
Copy link
Contributor Author

This proposed fix is outdated with the modelql rewrite in core 10.0.0

@mhuster23 mhuster23 closed this Nov 21, 2024
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.

2 participants