On Mon, 14 Apr 2025 08:02:34 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Could I have a review of a PR that improves the checks for label metadata? When running TestEventMetadata, I found some fields with incorrect capitalization. I fixed those, as well as some typos, and updated the filename from trace.xml to metadata.xml.
Testing: jdk/jdk/jfr
Thanks Erik
Makes sense. Only stylistic nits: Looks reasonable to me, thanks. test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 168:
166: Asserts.assertNotEquals(label, null, "Label not allowed to be null"); 167: Asserts.assertTrue(label.length() > 1, "Label must be at least two characters"); 168: Asserts.assertTrue(label.length() < 45, "Label should not exceed 45 characters, use description to explain");
Message suggests the condition should be `<= 45`? test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 177:
175: Asserts.assertTrue(isCapitalized(firstWord), "Label should capitalize first word"); 176: if (!isNumeric(lastWord)) { 177: Asserts.assertTrue(isCapitalized(lastWord), "Label should capitalize last word");
This looks like a heuristic that last word is very likely a noun, and should be capitalized then? Just curious. test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 181:
179: for (String word : words) { 180: Asserts.assertFalse(word.endsWith("-") || word.startsWith("-"), "Word in label should not start or end with hyphen"); 181: Asserts.assertTrue(word.length() != 0, "Label should not contain superflous whitespace");
"superfluous"? test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 194:
192: } 193: for (char c : label.toCharArray()) { 194: Asserts.assertTrue(isAllowedCharacter(c), "Label should only consist of letters, numbers, hyphens, parantheses or whitespace, found '" + c + "'");
"parentheses"? test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 212:
210: 211: private static boolean isShortCommonPreposition(String word) { 212: String[] prepositions = { "in", "on", "at", "by", "to", "of"};
Suggestion: String[] prepositions = { "in", "on", "at", "by", "to", "of" }; ------------- PR Review: https://git.openjdk.org/jdk/pull/24616#pullrequestreview-2763542035 Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24616#pullrequestreview-2764010341 PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041660467 PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041676636 PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041679340 PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041679801 PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041680301