elastic / elasticsearch-net

This strongly-typed, client library enables working with Elasticsearch. It is the official client maintained and supported by Elastic.
https://www.elastic.co/guide/en/elasticsearch/client/net-api/current/index.html
Apache License 2.0
11 stars 1.15k forks source link

8.15.6: TypeMappingDescriptor Dynamic Feedback #8346

Open niemyjski opened 2 months ago

niemyjski commented 2 months ago

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: https://github.com/FoundatioFx/Foundatio.Parsers/pull/84

flobernd commented 2 months ago

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.

github-actions[bot] commented 1 month ago

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 commented 1 month ago

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