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

Protect against related field issue #28

Closed
wants to merge 3 commits into from
Closed

Conversation

sposs
Copy link

@sposs sposs commented Jun 12, 2024

Hopefully fixes #24

@vdboor
Copy link
Contributor

vdboor commented Jul 9, 2024

Hi! Thanks for offering this fix. It looks like a good first start. I wonder, would it be possible to have some kind of unit test or example for this? That would make it easier to guarantee this is the right fix. TIA!

@sposs
Copy link
Author

sposs commented Jul 9, 2024

I've added a test that fails if the patch is not added. Moreover, I see that the previous test cases passed because the feature Restaurant has a field 'name' and the city also has a field 'name', and so there is collision between the field names. I've thus added a field that does not exist in the Restaurant to ensure the resolution of related.

@sposs
Copy link
Author

sposs commented Jul 9, 2024

Last commit needed because your pre-commit hooks are a little too strict on the line lengths...

Copy link
Contributor

@vdboor vdboor left a comment

Choose a reason for hiding this comment

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

Sorry I missed this one, looks good! Thanks!

@vdboor
Copy link
Contributor

vdboor commented Aug 29, 2024

I've merged this manually with rebasing (to resolve conflicts). Your changes are in 7294678 and are released as 1.4.1 :-)

I've updated our pre-commit-config too in main. It's quite odd that it triggered for you, as pre-commit run -a doesn't report anything here.

@vdboor vdboor closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

GetCapabilities does not work with dotted model_attribute value
2 participants