Skip to content

Edits to text in API Conventions docs#39001

Merged
danielmitterdorfer merged 3 commits intoelastic:6.6from
dmeiss:patch-9
Feb 20, 2019
Merged

Edits to text in API Conventions docs#39001
danielmitterdorfer merged 3 commits intoelastic:6.6from
dmeiss:patch-9

Conversation

@dmeiss
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

elasticmachine commented Feb 18, 2019

Pinging @elastic/es-core-infra

@danielmitterdorfer
Copy link
Copy Markdown
Member

danielmitterdorfer commented Feb 18, 2019

@elasticmachine test this please

danielmitterdorfer
danielmitterdorfer requested changes Feb 18, 2019
View reviewed changes
Copy link
Copy Markdown
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?

docs/reference/api-conventions.asciidoc Outdated

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

@danielmitterdorfer danielmitterdorfer Feb 18, 2019

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

elasticsearch/server/src/main/java/org/elasticsearch/common/unit/SizeValue.java

Lines 170 to 197 in 869de3f

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.

@dmeiss
Made requested change on “s” multiplier
e79d2dc 
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
Copy Markdown
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
Copy Markdown
Member

danielmitterdorfer commented Feb 18, 2019

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
Copy Markdown
Contributor Author

dmeiss commented Feb 18, 2019

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

@dmeiss
Copy link
Copy Markdown
Contributor Author

dmeiss commented Feb 18, 2019

Went ahead & removed the "single" line

danielmitterdorfer
danielmitterdorfer approved these changes Feb 19, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

danielmitterdorfer commented Feb 19, 2019

@elasticmachine test this please

@danielmitterdorfer danielmitterdorfer merged commit 208f447 into elastic:6.6 Feb 20, 2019
danielmitterdorfer pushed a commit that referenced this pull request Feb 20, 2019
@dmeiss @danielmitterdorfer
Edits to text in API Conventions docs (#39001)
19082ae
danielmitterdorfer pushed a commit that referenced this pull request Feb 20, 2019
@dmeiss @danielmitterdorfer
Edits to text in API Conventions docs (#39001)
5b74be1
danielmitterdorfer pushed a commit that referenced this pull request Feb 20, 2019
@dmeiss @danielmitterdorfer
Edits to text in API Conventions docs (#39001)
dc1ed80
danielmitterdorfer pushed a commit that referenced this pull request Feb 20, 2019
@dmeiss @danielmitterdorfer
Edits to text in API Conventions docs (#39001)
be7fa87
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 20, 2019
@jasontedor
Merge branch 'master' into aws-sdk-jackson-databind-2.8.11.3
7948458 
* 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
@jasontedor
Merge branch 'master' into ingest-geoip-jackson-databind-2.8.11.3
37c46e0 
* 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
@dmeiss @weizijun
Edits to text in API Conventions docs (elastic#39001)
6cb8b8f
weizijun pushed a commit to weizijun/elasticsearch that referenced this pull request Feb 22, 2019
@dmeiss @weizijun
Edits to text in API Conventions docs (elastic#39001)
41cf11b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danielmitterdorfer danielmitterdorfer danielmitterdorfer approved these changes

Assignees

No one assigned

Labels

:Core/Infra/REST API REST infrastructure and utilities >docs General docs changes v6.6.2 v6.7.0 v7.0.0-rc2 v7.2.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dmeiss @elasticmachine @danielmitterdorfer @jakelandis