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

chore: allow mixing meta while merging Body.Data #111

Merged

Conversation

nekrich
Copy link
Contributor

@nekrich nekrich commented Sep 20, 2023

Ignore irrelevant generic types when merging Document.Body.Data.

I have a paginated request, and the response data has no meta and links because it’s irrelevant in the case when Meta is NoMetadata.

Also, made it possible to merge data with different API descriptions and errors.

@nekrich nekrich force-pushed the chore/allow-mixed-meta-when-merging branch 2 times, most recently from af4a9bd to 19b3c9d Compare September 20, 2023 16:13
Ignore irrelevant generic types when merging Document.Body.Data.

### Reasoning

I have a paginated request and response data has no meta and links, because it’s irrelevant for me.
@nekrich nekrich force-pushed the chore/allow-mixed-meta-when-merging branch from 19b3c9d to b9cb23d Compare September 20, 2023 16:25
Copy link
Owner

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

This looks like a nice improvement.

@mattpolzin
Copy link
Owner

mattpolzin commented Mar 1, 2024

Hmm, so I see the logic behind not caring about the OtherDescription or OtherError when merging the Document.Body because those are not stored in the Body. On the other hand, your changes to the merging implementation where MetaType == NoMetadata and LinksType == NoLinks seem less intuitive to me; they merge in a document body that does have metadata and/or links by throwing away the metadata and links from the body being merged in. I think the more intuitive result would be to keep the metadata and links from the document being merged in.

…rged-into document body has no metadata or links.
@mattpolzin
Copy link
Owner

I think I've maintained the spirit of this PR with my last change so I am going to merge this as it's sat open for far longer than I intended it to. Please let me know if I've mucked things up for you by changing the resulting type for the document merge.

@mattpolzin mattpolzin merged commit 399ec4c into mattpolzin:main Mar 1, 2024
25 checks passed
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