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

8.15.6: TypeMappingDescriptor Dynamic Feedback #8346

Open
niemyjski opened this issue Sep 13, 2024 · 3 comments
Open

8.15.6: TypeMappingDescriptor Dynamic Feedback #8346

niemyjski opened this issue Sep 13, 2024 · 3 comments
Labels

Comments

@niemyjski
Copy link
Contributor

Elastic.Clients.Elasticsearch version: 8.15.6

Elasticsearch version: 8.15.1

.NET runtime version: 8.x

Operating system version: Any

Description of the problem including expected versus actual behavior:

The new generated api has some pain points on upgrading I felt might have been overlooked but would be easier on those upgrading.

The existing mappings had .Dynamic() but the new one requires you to specify an enum .Dynamic(DynamicMapping.True). I don't know why this couldn't just be the default. You are not going to do .Dynamic(DynamicMapping.None) as you would just omit it. I get there are other enum values but seems like those could be specified. We have tons and tons of theses that we had to bulk replace.

The same feedback goes for other areas like .TrackTotalHits(true) -> .TrackTotalHits(new TrackHits(true)))

Expected behavior
Smart defaults would go a long way to not breaking existing api's.

Reference: FoundatioFx/Foundatio.Parsers#84

@flobernd
Copy link
Member

Hi @niemyjski ,

I don't know why this couldn't just be the default

For the Dynamic case:

Because the code generator does not know that this might be a good default to use 🙂

In our specification, we currently do not maintain "client defaults". This would be a requirement for generating "smart" code like the one you mentioned.

For TrackTotalHits:

This is similar to #8345 and boils down to the same issue #8347.

Copy link
Contributor

This issue is stale because it has been open 5 days with no activity. Remove stale label or comment or this will be closed in 2 days.

@niemyjski
Copy link
Contributor Author

Couldn't we just define a default constructor in a partial class?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants