-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Addition of [Obsolete(...)] attributes to 5.x #3023
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a small note. For read only classes that are part of the response but the actual property access path is unchanged we should not add Obsolete
's i reckon,
@@ -115,6 +115,7 @@ internal static Error Create(IDictionary<string, object> dict, IJsonSerializerSt | |||
: $"Type: {this.Type} Reason: \"{this.Reason}\" CausedBy: \"{this.CausedBy}\""; | |||
} | |||
|
|||
[Obsolete("Removed in 6.0.")] | |||
public class CausedBy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While technically true this is only used through reading server errors, when upgrading no code for the property access into this needs to be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather leave this out to reduce noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some general comments.
The large issues that the Obsolete
attributes flag are:
- removal of
IncludeInAll
in field mappings, as part of the removal of the_all
field - removal of stored search templates related methods
- removal of file based scripting
Are these better served as main documentation sections in https://www.elastic.co/guide/en/elasticsearch/client/net-api/current/nest-breaking-changes.html? thoughts @codebrain @Mpdreamz ?
@@ -35,7 +35,7 @@ public interface ISignificantTermsAggregation : IBucketAggregation | |||
/// number of hits | |||
/// </summary> | |||
[JsonIgnore] | |||
[Obsolete("Use MinimumDocumentCountAsLong. Fixed in NEST 6.x")] | |||
[Obsolete("Use MinimumDocumentCountAsLong. Removed in NEST 6.x")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think fixed is a better description IMO; MinimumDocumentCount
is a long?
in 6.x and MinimumDocumentCountAsLong
is removed, since it served only as a non-breaking BWC change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -131,7 +131,7 @@ public class SignificantTermsAggregation : BucketAggregationBase, ISignificantTe | |||
/// <inheritdoc /> | |||
public int? ShardSize { get; set; } | |||
|
|||
[Obsolete("Use MinimumDocumentCountAsLong. Fixed in NEST 6.x")] | |||
[Obsolete("Use MinimumDocumentCountAsLong. Removed in NEST 6.x")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think fixed is a better description IMO; MinimumDocumentCount is a long? in 6.x and MinimumDocumentCountAsLong is removed, since it served only as a non-breaking BWC change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -188,7 +188,7 @@ public class SignificantTermsAggregationDescriptor<T> | |||
|
|||
int? ISignificantTermsAggregation.ShardSize { get; set; } | |||
|
|||
[Obsolete("Use MinimumDocumentCountAsLong. Fixed in NEST 6.x")] | |||
[Obsolete("Use MinimumDocumentCountAsLong. Removed in NEST 6.x")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think fixed is a better description IMO; MinimumDocumentCount is a long? in 6.x and MinimumDocumentCountAsLong is removed, since it served only as a non-breaking BWC change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -12,6 +13,7 @@ public interface IHunspellTokenFilter : ITokenFilter | |||
/// If true, dictionary matching will be case insensitive. | |||
/// </summary> | |||
[JsonProperty("ignore_case")] | |||
[Obsolete("Scheduled to be removed in 6.0")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it scheduled still for a later Elasticsearch 6.x, or been removed in Elasticsearch 6.0? Can we advise an alternative approach e.g. using a lowercase
token filter before hunspell
token filter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added help and changed to 6.x.
/// Takes precedence over the global <see cref="DefaultIndex"/> | ||
/// </summary> | ||
[Obsolete("Removed in 6.0.")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined not to Obsolete
this as it is likely to be noisy for a lot of users, since it is a commonly used method. Perhaps an XML comment instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to ///remarks
using Newtonsoft.Json; | ||
|
||
namespace Nest | ||
{ | ||
[Obsolete("Removed in NEST 6.x")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can help users here by suggesting that they move to using suggesters on the Search()
endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
[JsonConverter(typeof(SuggestRequestJsonConverter))] | ||
public partial interface ISuggestRequest | ||
{ | ||
string GlobalText { get; set; } | ||
ISuggestContainer Suggest { get; set; } | ||
} | ||
|
||
[Obsolete("Removed in NEST 6.x")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can help users here by suggesting that they move to using suggesters on the Search()
endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
public partial class SuggestRequest | ||
{ | ||
public string GlobalText { get; set; } | ||
public ISuggestContainer Suggest { get; set; } | ||
} | ||
|
||
[Obsolete("Removed in NEST 6.x")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can help users here by suggesting that they move to using suggesters on the Search()
endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
public partial interface ISuggestRequest<T> : ISearchRequest { } | ||
|
||
[Obsolete("Removed in NEST 6.x")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can help users here by suggesting that they move to using suggesters on the Search()
endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
public partial class SuggestRequest<T> | ||
{ | ||
public string GlobalText { get; set; } | ||
public ISuggestContainer Suggest { get; set; } | ||
} | ||
|
||
[DescriptorFor("Suggest")] | ||
[Obsolete("Removed in NEST 6.x")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can help users here by suggesting that they move to using suggesters on the Search()
endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@russcam - There are a few overriding changes (at least the 3 you called out) where a lot of noise was generated. Probably best for documentation. |
Early warning for 5.x users