Skip to content

Conversation

Mpdreamz
Copy link
Member

fix for #2493

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.

Left a couple of comments, LGTM otherwise.

@@ -0,0 +1,162 @@
namespace Nest.CommonOptions.DateFormat
Copy link
Contributor

Choose a reason for hiding this comment

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

namespace should be "Nest"

public static class DateFormat
{
///<summary>A formatter for the number of milliseconds since the epoch. Note, that this timestamp is subject to the limits of a Java Long.MIN_VALUE and Long.MAX_VALUE.</summary>
public const string epoch_millis = "epoch_mills";
Copy link
Contributor

Choose a reason for hiding this comment

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

Strong opinions about using the original names v.s. idiomatic .NET Pascal casing?

Copy link
Member Author

Choose a reason for hiding this comment

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

kinda :) EpochMillis(seconds) not that bad but turning strict_t_time into StrictTTIme :/ their is litttle guessing or ambiguity in keeping the casing which is a big plus over dogmatic casing, What do you guys reckon?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm cool with using the original names.

@Mpdreamz Mpdreamz merged commit ca9fdd2 into master Jan 25, 2017
Mpdreamz added a commit that referenced this pull request Jan 25, 2017
Conflicts:
	src/Nest/Nest.csproj
@Mpdreamz
Copy link
Member Author

backported to 5.x and 2.x

@Mpdreamz Mpdreamz deleted the feaure/dateformat-consts branch January 25, 2017 10:07
awelburn pushed a commit to Artesian/elasticsearch-net that referenced this pull request Nov 6, 2017
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