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

Edits to text in API Conventions docs #39001

Merged
merged 3 commits into from
Feb 20, 2019
Merged

Conversation

dmeiss
Copy link
Contributor

@dmeiss dmeiss commented Feb 16, 2019

Minor edits to phrasing, punctuation, capitalization, and formatting.

@danielmitterdorfer danielmitterdorfer added >docs General docs changes :Core/Infra/REST API REST infrastructure and utilities labels Feb 18, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@danielmitterdorfer
Copy link
Member

@elasticmachine test this please

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks for these fixes @dmeiss. The changes look mostly fine to me but I left one remark about an incorrect multiplier. Can you please remove the "s" multiplier?

@@ -526,7 +528,7 @@ If one of these quantities is large we'll print it out like 10m for 10,000,000 o
when we mean 87 though. These are the supported multipliers:

[horizontal]
``:: Single
`s`:: Single
Copy link
Member

Choose a reason for hiding this comment

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

The s suffix is not recognized by Elasticsearch (see also

public static SizeValue parseSizeValue(String sValue) throws ElasticsearchParseException {
return parseSizeValue(sValue, null);
}
public static SizeValue parseSizeValue(String sValue, SizeValue defaultValue) throws ElasticsearchParseException {
if (sValue == null) {
return defaultValue;
}
long singles;
try {
if (sValue.endsWith("k") || sValue.endsWith("K")) {
singles = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * SizeUnit.C1);
} else if (sValue.endsWith("m") || sValue.endsWith("M")) {
singles = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * SizeUnit.C2);
} else if (sValue.endsWith("g") || sValue.endsWith("G")) {
singles = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * SizeUnit.C3);
} else if (sValue.endsWith("t") || sValue.endsWith("T")) {
singles = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * SizeUnit.C4);
} else if (sValue.endsWith("p") || sValue.endsWith("P")) {
singles = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * SizeUnit.C5);
} else {
singles = Long.parseLong(sValue);
}
} catch (NumberFormatException e) {
throw new ElasticsearchParseException("failed to parse [{}]", e, sValue);
}
return new SizeValue(singles, SizeUnit.SINGLE);
}
) so this needs to be an empty string.

Deleted the “s”.

Is it OK that it then displays as two backtick characters? Or does it maybe need to be two single (or double) quotes to indicate an empty string, e.g. “”? 

I’m an editor by trade, not a coder — if that wasn’t already obvious. =).  Thanks for your patience.
@dmeiss
Copy link
Contributor Author

dmeiss commented Feb 18, 2019

Made requested changes. Have a question though about how that empty string should be represented. It’s currently `` but I’m wondering if it should be “” instead.

@danielmitterdorfer
Copy link
Member

Have a question though about how that empty string should be represented. It’s currently `` but I’m wondering if it should be “” instead.

On second thought we should just remove the entire line. I doubt that it helps anybody if we document that if no multiplier is specified, we treat the value as is. I think this is expected behavior anyway.

@dmeiss
Copy link
Contributor Author

dmeiss commented Feb 18, 2019

I agree. Would you like me to go ahead & remove it?

@dmeiss
Copy link
Contributor Author

dmeiss commented Feb 18, 2019

Went ahead & removed the "single" line

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me.

@danielmitterdorfer
Copy link
Member

@elasticmachine test this please

@danielmitterdorfer danielmitterdorfer merged commit 208f447 into elastic:6.6 Feb 20, 2019
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 20, 2019
* master:
  Mute failing CCR retention lease unfollow test
  Add support for ccr follow info api to HLRC. (elastic#39115)
  Do not create the missing index when invoking getRole (elastic#39039)
  Relax history check in ShardFollowTaskReplicationTests (elastic#39162)
  Add retention leases replication tests (elastic#38857)
  Edits to text in Phrase Suggester doc (elastic#38966)
  Edits to text in API Conventions docs (elastic#39001)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 20, 2019
* master:
  Mute failing CCR retention lease unfollow test
  Add support for ccr follow info api to HLRC. (elastic#39115)
  Do not create the missing index when invoking getRole (elastic#39039)
  Relax history check in ShardFollowTaskReplicationTests (elastic#39162)
  Add retention leases replication tests (elastic#38857)
  Edits to text in Phrase Suggester doc (elastic#38966)
  Edits to text in API Conventions docs (elastic#39001)
weizijun pushed a commit to weizijun/elasticsearch that referenced this pull request Feb 22, 2019
weizijun pushed a commit to weizijun/elasticsearch that referenced this pull request Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants