Skip to content

Conversation

@Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Dec 8, 2017

… in favor of Id

Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

LGTM. Left some comments around makring as Obsolete in 6.x, as opposed to removing and adding Obsolete attributes to 5.x

public IndexedScriptDescriptor Id(string id) => new IndexedScriptDescriptor(id);

[Obsolete("Indexed() sets a property named id, this is confusing and thats why we intent to remove this in NEST 7.x please use Id()")]
public IndexedScriptDescriptor Indexed(string id) => new IndexedScriptDescriptor(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not just remove this and mark as [Obsolete] in 5.x?


public IndexedScriptConditionDescriptor Id(string id) => new IndexedScriptConditionDescriptor(id);
[Obsolete("Indexed() sets a property named id, this is confusing and thats why we intent to remove this in NEST 7.x please use Id()")]
public IndexedScriptConditionDescriptor Indexed(string id) => new IndexedScriptConditionDescriptor(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not just remove this and mark as [Obsolete] in 5.x?

public IndexedScriptConditionDescriptor Indexed(string id) => new IndexedScriptConditionDescriptor(id);

[Obsolete("Inline is being deprecated for Source and will be removed in Elasticsearch 7.0")]
public InlineScriptConditionDescriptor Inline(string script) => new InlineScriptConditionDescriptor(script);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not just remove this and mark as [Obsolete] in 5.x?

@russcam russcam force-pushed the fix/file-scripts branch 2 times, most recently from acda1e6 to 0c6ff23 Compare December 14, 2017 04:52
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.

3 participants