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

Add Discriminator mapping #752

Merged
merged 3 commits into from
Sep 22, 2023
Merged

Conversation

1lutz
Copy link
Contributor

@1lutz 1lutz commented Sep 4, 2023

I need to set the mapping property for the discriminator of oneOfs.

Resolves #738

Copy link
Contributor

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

this isnt where the mapping exists. The Discriminator only holds the name of the property which will be used to check whether the alternatives have compatible structures.
I suggest closing this PR and explaining what you need more on the issue.

@juhaku
Copy link
Owner

juhaku commented Sep 12, 2023

I agree there is certainly a need for clarification about what is needed to be done. I sort of get what is the wished outcome of the implementation but the current one here is not complete in itself. Just for starters there are no tests to test the correctness nor there are macro support thus using this would work only when types are manually constructed.

I added more elaborate message here #738 (comment)

@@ -284,6 +284,10 @@ pub struct Discriminator {
/// Defines a discriminator property name which must be found within all composite
/// objects.
pub property_name: String,

// An object to hold mappings between payload values and schema names or references.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to triple slash and expand the comment so it is more suitable for public documentation?
It needs to state that it can only be populated manually - i.e. there is no macro support, and there is no validation of the mapping.

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 edited the comment and added a test here: c2834c5

@jayvdb
Copy link
Contributor

jayvdb commented Sep 14, 2023

Given the work you are doing at https://github.com/1lutz/geoengine/blob/5e17832833fefc3a0915df4de5ee68bb3c8cee5d/services/src/util/apidoc.rs#L168 , I can see the value of getting this in, even if utoipa doesnt do anything with this mapping field.

And you could grab a subset of that code and convert it to be a unit test quite easily...?

@juhaku
Copy link
Owner

juhaku commented Sep 15, 2023

Yeah I agree as I also described here #738 (comment). That is unfortunate at the moment that there is a need for such manual processing.

I think we can let this in without further additions but the doc comment could be improved as @jayvdb mentioned above.

Copy link
Owner

@juhaku juhaku left a comment

Choose a reason for hiding this comment

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

Super 🎉

@juhaku juhaku merged commit 6dc73eb into juhaku:master Sep 22, 2023
5 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.

Manually specify Discriminator Mapping
3 participants