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

Updates navigation property expansion logic #160

Merged
merged 11 commits into from
Feb 10, 2022

Conversation

irvinesunday
Copy link
Contributor

Fixes #123

This PR proposes:

  • Updating the expansion logic for containment navigation properties to evaluate based on whether the navigation property has been visited before and not the navigation property type.

NB: This approach expands on all containment navigation properties. This ends up exploding the number of paths significantly.

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.

does this also replace #152?

You probably need to update the integration test files as well.

@baywet baywet added this to the 1.0.10 milestone Jan 17, 2022
@irvinesunday
Copy link
Contributor Author

does this also replace #152?

You probably need to update the integration test files as well.

@baywet this PR does not replace that #152. This one just deals with the expansion logic of containment navigation properties, the other issue has to do with "turning off" containment of some navigation properties without setting ContainsTarget=true to false

@baywet
Copy link
Member

baywet commented Jan 17, 2022

ok thanks for confirming. I believe then that #158 does what this does and more, there's just a last minute bug in there I need to address, but please have a look at the code and tell me what you think.

@irvinesunday
Copy link
Contributor Author

ok thanks for confirming. I believe then that #158 does what this does and more, there's just a last minute bug in there I need to address, but please have a look at the code and tell me what you think.

I'm unable to run the branch on that PR due to the bug, but from a quick look at the expansion logic of containment navigation properties I don't see where it has modified the current experience.

@irvinesunday irvinesunday self-assigned this Jan 19, 2022
Don't check on the defining type to determine whether or not to expand, but on whether the navigation property has already been navigated in the path
@baywet
Copy link
Member

baywet commented Jan 21, 2022

for sanity: this was discussed offline but this PR doesn't overlap with #160, both are needed. It however adds a lot of path items and is probably not yet ready until #152 gets resolved.

baywet
baywet previously approved these changes Feb 9, 2022
@xuzhg
Copy link
Contributor

xuzhg commented Feb 9, 2022

looks good to me.

@irvinesunday irvinesunday merged commit 4294381 into master Feb 10, 2022
@irvinesunday irvinesunday deleted the is/fix-nav-prop-expand-logic branch February 10, 2022 04:40
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.

Missing navigation path items for navigation properties with contains target
3 participants