From 416ef861b50b454dba318ea43eb29bb58c9ef71d Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Fri, 10 Nov 2023 12:44:14 -0500 Subject: [PATCH] More code review suggestions. --- .../cql/cql2elm/LibraryBuilder.java | 72 +++++++------------ .../cql/cql2elm/model/MatchType.java | 17 ----- .../cqframework/cql/cql2elm/HidingTests.java | 27 ++++--- .../cql/cql2elm/TranslationTests.java | 22 +++--- .../cql/cql2elm/qicore/v411/BaseTest.java | 4 +- .../cql/cql2elm/qicore/v500/BaseTest.java | 4 +- 6 files changed, 54 insertions(+), 92 deletions(-) delete mode 100644 Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java index 53ca1e965..b0097c575 100644 --- a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java +++ b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java @@ -2344,54 +2344,43 @@ public Expression resolveIdentifier(String identifier, boolean mustResolve) { return null; } - private static String formatMatchedMessage(MatchType matchType) { - switch (matchType) { - case EXACT: - return " with exact case matching."; - case CASE_IGNORED: - return " with case insensitive matching."; - default: - return " with invalid MatchType."; - } - } - private static String lookupElementWarning(Object element) { // TODO: this list is not exhaustive and may need to be updated if (element instanceof ExpressionDef) { - return "[%s] resolved as an expression definition"; + return "An expression"; } else if (element instanceof ParameterDef) { - return "[%s] resolved as a parameter"; + return "A parameter"; } else if (element instanceof ValueSetDef) { - return "[%s] resolved as a value set"; + return "A valueset"; } else if (element instanceof CodeSystemDef) { - return "[%s] resolved as a code system"; + return "A codesystem"; } else if (element instanceof CodeDef) { - return "[%s] resolved as a code"; + return "A code"; } else if (element instanceof ConceptDef) { - return "[%s] resolved as a concept"; + return "A concept"; } else if (element instanceof IncludeDef) { - return "[%s] resolved as a library"; + return "An include"; } else if (element instanceof AliasedQuerySource) { - return "[%s] resolved as an alias of a query"; + return "An alias"; } else if (element instanceof LetClause) { - return "[%s] resolved as a let of a query"; + return "A let"; } else if (element instanceof OperandDef) { - return "[%s] resolved as an operand to a function"; + return "An operand"; } else if (element instanceof UsingDef) { - return "[%s] resolved as a using definition"; + return "A using"; } //default message if no match is made: - return "[%s] resolved more than once: " + ((element != null) ? element.getClass() : "[null]"); + return "An [unknown structure]"; } /** @@ -2998,33 +2987,26 @@ private DataType getExpressionDefResultType(ExpressionDef expressionDef) { * * @param identifier The identifier belonging to the parameter, expression, function, alias, etc, to be evaluated. * @param onlyOnce Special case to deal with overloaded functions, which are out scope for hiding. - * @param element The construct element, for {@link ExpressionRef}. + * @param element The construct element, for example {@link ExpressionRef}. */ void pushIdentifierForHiding(String identifier, boolean onlyOnce, Element element) { - final MatchType matchType = identifiersToCheckForHiding.stream() - .map(innerIdentifier -> { - if (innerIdentifier.equals(identifier)) { - return MatchType.EXACT; + final boolean hasRelevantMatch = identifiersToCheckForHiding.stream() + .filter(innerIdentifier -> innerIdentifier.equalsIgnoreCase(identifier)) + .peek(matchedIdentifier -> { + if (onlyOnce) { + return; } - if (innerIdentifier.equalsIgnoreCase(identifier)) { - return MatchType.CASE_IGNORED; - } - - return MatchType.NONE; - }) - .filter(innerMatchType -> MatchType.NONE != innerMatchType) - .findFirst() - .orElse(MatchType.NONE); - - if (MatchType.NONE != matchType && ! onlyOnce) { - final String message = String.format("Identifier hiding detected: Identifier for identifiers: %s%s", - String.format(lookupElementWarning(element), identifier), - formatMatchedMessage(matchType)+ "\n"); - reportWarning(message, element); - } + final boolean isExactMatch = matchedIdentifier.equals(identifier); + final String elementString = lookupElementWarning(element); + final String message= isExactMatch + ? String.format("%s identifier [%s] is hiding another identifier of the same name. %n", elementString, identifier) + : String.format("Are you sure you mean to use %s identifier [%s], instead of [%s]? %n", elementString.toLowerCase(), identifier, matchedIdentifier) ; - if (! onlyOnce || MatchType.NONE == matchType) { + reportWarning(message, element); + }).findAny() + .isPresent(); + if (! onlyOnce || ! hasRelevantMatch) { identifiersToCheckForHiding.push(identifier); } } diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java deleted file mode 100644 index 13ed734b9..000000000 --- a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java +++ /dev/null @@ -1,17 +0,0 @@ -package org.cqframework.cql.cql2elm.model; - -public enum MatchType { - EXACT, CASE_IGNORED, NONE; - - public static MatchType resolveMatchType(String val, String checkVal) { - if (val.equals(checkVal)) { - return EXACT; - } - - if (val.equalsIgnoreCase(checkVal)) { - return CASE_IGNORED; - } - - return NONE; - } -} \ No newline at end of file diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java index 43c121645..d3d76358b 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java @@ -18,7 +18,7 @@ public void testCaseInsensitiveWarning() throws IOException { final List warnings = translator.getWarnings(); assertThat(warnings.toString(), translator.getWarnings().size(), is(1)); final Set warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toSet()); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [patients] resolved as an expression definition with case insensitive matching.\n")); + assertThat(warningMessages, contains("Are you sure you mean to use an expression identifier [patients], instead of [Patients]? \n")); } @Test @@ -28,7 +28,7 @@ public void testHiddenIdentifierFromReturn() throws IOException { assertThat(warnings.toString(), translator.getWarnings().size(), is(1)); final Set warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toSet()); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [var] resolved as a let of a query with exact case matching.\n")); + assertThat(warningMessages, contains("A let identifier [var] is hiding another identifier of the same name. \n")); } @Test @@ -49,10 +49,7 @@ public void testHidingUnionWithSameAliasEachHides() throws IOException { final List distinct = translator.getWarnings().stream().map(Throwable::getMessage).distinct().collect(Collectors.toList()); assertThat(distinct.size(), is(1)); - - final String first = "Identifier hiding detected: Identifier for identifiers: [IWantToBeHidden] resolved as an alias of a query with exact case matching.\n"; - - assertThat(distinct, containsInAnyOrder(first)); + assertThat(distinct, contains("An alias identifier [IWantToBeHidden] is hiding another identifier of the same name. \n")); } @Test @@ -69,7 +66,7 @@ public void testSoMuchNestingHidingSimple() throws IOException { final List warnings = translator.getWarnings(); assertThat(warnings.toString(), translator.getWarnings().size(), is(1)); - assertThat(warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()), containsInAnyOrder("Identifier hiding detected: Identifier for identifiers: [SoMuchNesting] resolved as an alias of a query with exact case matching.\n")); + assertThat(warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()), containsInAnyOrder("An alias identifier [SoMuchNesting] is hiding another identifier of the same name. \n")); } @Test @@ -84,8 +81,8 @@ public void testSoMuchNestingHidingComplex() throws IOException { assertThat(distinct.size(), is(2)); - final String first = "Identifier hiding detected: Identifier for identifiers: [SoMuchNesting] resolved as an alias of a query with exact case matching.\n"; - final String second = "Identifier hiding detected: Identifier for identifiers: [SoMuchNesting] resolved as a let of a query with exact case matching.\n"; + final String first = "An alias identifier [SoMuchNesting] is hiding another identifier of the same name. \n"; + final String second = "A let identifier [SoMuchNesting] is hiding another identifier of the same name. \n"; assertThat(distinct, containsInAnyOrder(first, second)); } @@ -97,7 +94,7 @@ public void testHidingLetAlias() throws IOException { final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); assertThat(warningMessages.toString(), translator.getWarnings().size(), is(1)); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [Alias] resolved as a let of a query with exact case matching.\n")); + assertThat(warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()), containsInAnyOrder("A let identifier [Alias] is hiding another identifier of the same name. \n")); } @Test @@ -109,7 +106,7 @@ public void testHiddenIdentifierArgumentToAlias() throws IOException { .stream() .map(Throwable::getMessage) .collect(Collectors.toList()), - contains("Identifier hiding detected: Identifier for identifiers: [testOperand] resolved as an alias of a query with exact case matching.\n")); + contains("An alias identifier [testOperand] is hiding another identifier of the same name. \n")); } @Test @@ -124,7 +121,7 @@ public void testHidingFunctionDefinitionWithOverloads() throws IOException { final List warnings = translator.getWarnings(); final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); assertThat(warningMessages.toString(), warnings.size(), is(1)); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [IWantToBeHidden] resolved as an alias of a query with exact case matching.\n")); + assertThat(warningMessages, contains("An alias identifier [IWantToBeHidden] is hiding another identifier of the same name. \n")); } @Test @@ -133,7 +130,7 @@ public void testHidingParameterDefinition() throws IOException { final List warnings = translator.getWarnings(); final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); assertThat(warningMessages.toString(), warnings.size(), is(1)); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [Measurement Period] resolved as an alias of a query with exact case matching.\n")); + assertThat(warningMessages, contains("An alias identifier [Measurement Period] is hiding another identifier of the same name. \n")); } @Test @@ -142,7 +139,7 @@ public void testHidingIncludeDefinition() throws IOException { final List warnings = translator.getWarnings(); final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); assertThat(warningMessages.toString(), warnings.size(), is(1)); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [FHIRHelpers] resolved as an alias of a query with exact case matching.\n")); + assertThat(warningMessages, contains("An alias identifier [FHIRHelpers] is hiding another identifier of the same name. \n")); } @Test @@ -153,6 +150,6 @@ public void testHidingCommaMissingInListConstruction() throws IOException { assertThat(warningMessages.toString(), warnings.size(), is(2)); final List distinctWarningMessages = warningMessages.stream().distinct().collect(Collectors.toList()); assertThat(distinctWarningMessages.toString(), distinctWarningMessages.size(), is(1)); - assertThat(distinctWarningMessages, contains("Identifier hiding detected: Identifier for identifiers: [5] resolved as an alias of a query with exact case matching.\n")); + assertThat(distinctWarningMessages, contains("An alias identifier [5] is hiding another identifier of the same name. \n")); } } diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java index 4704378cb..476b0cf4a 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java @@ -292,17 +292,17 @@ public void testHidingVariousUseCases() throws IOException { assertThat(warningMessages.toString(), distinct.size(), is(11)); - final String hidingDefinition = "Identifier hiding detected: Identifier for identifiers: [Definition] resolved as an alias of a query with exact case matching.\n"; - final String hidingVarLet = "Identifier hiding detected: Identifier for identifiers: [var] resolved as a let of a query with exact case matching.\n"; - final String hidingContextValueSet = "Identifier hiding detected: Identifier for identifiers: [ValueSet] resolved as an alias of a query with exact case matching.\n"; - final String hidingLetValueSet = "Identifier hiding detected: Identifier for identifiers: [ValueSet] resolved as a let of a query with exact case matching.\n"; - final String hidingContextCode = "Identifier hiding detected: Identifier for identifiers: [Code] resolved as an alias of a query with exact case matching.\n"; - final String hidingLetCode = "Identifier hiding detected: Identifier for identifiers: [Code] resolved as a let of a query with exact case matching.\n"; - final String hidingContextCodeSystem = "Identifier hiding detected: Identifier for identifiers: [CodeSystem] resolved as an alias of a query with exact case matching.\n"; - final String hidingLetCodeSystem = "Identifier hiding detected: Identifier for identifiers: [CodeSystem] resolved as a let of a query with exact case matching.\n"; - final String hidingContextFhir = "Identifier hiding detected: Identifier for identifiers: [FHIR] resolved as an alias of a query with exact case matching.\n"; - final String hidingLetFhir = "Identifier hiding detected: Identifier for identifiers: [FHIR] resolved as a let of a query with exact case matching.\n"; - final String hidingAliasLet = "Identifier hiding detected: Identifier for identifiers: [Alias] resolved as a let of a query with exact case matching.\n"; + final String hidingDefinition = "An alias identifier [Definition] is hiding another identifier of the same name. \n"; + final String hidingVarLet = "A let identifier [var] is hiding another identifier of the same name. \n"; + final String hidingContextValueSet = "An alias identifier [ValueSet] is hiding another identifier of the same name. \n"; + final String hidingLetValueSet = "A let identifier [ValueSet] is hiding another identifier of the same name. \n"; + final String hidingContextCode = "An alias identifier [Code] is hiding another identifier of the same name. \n"; + final String hidingLetCode = "A let identifier [Code] is hiding another identifier of the same name. \n"; + final String hidingContextCodeSystem = "An alias identifier [CodeSystem] is hiding another identifier of the same name. \n"; + final String hidingLetCodeSystem = "A let identifier [CodeSystem] is hiding another identifier of the same name. \n"; + final String hidingContextFhir = "An alias identifier [FHIR] is hiding another identifier of the same name. \n"; + final String hidingLetFhir = "A let identifier [FHIR] is hiding another identifier of the same name. \n"; + final String hidingAliasLet = "A let identifier [Alias] is hiding another identifier of the same name. \n"; assertThat(distinct, containsInAnyOrder(hidingDefinition, hidingVarLet, hidingContextValueSet, hidingLetValueSet, hidingContextCode, hidingLetCode, hidingContextCodeSystem, hidingLetCodeSystem, hidingContextFhir, hidingLetFhir, hidingAliasLet)); } diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java index 213b2800b..66a270691 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java @@ -29,8 +29,8 @@ public void testAuthoringPatterns() throws IOException { assertThat(distinct.size(), is(2)); - final String first = "Identifier hiding detected: Identifier for identifiers: [Diabetes] resolved as an alias of a query with exact case matching.\n"; - final String second = "Identifier hiding detected: Identifier for identifiers: [Application of Intermittent Pneumatic Compression Devices (IPC)] resolved as a value set with case insensitive matching.\n"; + final String first = "An alias identifier [Diabetes] is hiding another identifier of the same name. \n"; + final String second = "Are you sure you mean to use a valueset identifier [Application of Intermittent Pneumatic Compression Devices (IPC)], instead of [Application of intermittent pneumatic compression devices (IPC)]? \n"; assertThat(distinct, containsInAnyOrder(first, second)); } diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java index d9c8acc58..7b5af139a 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java @@ -38,8 +38,8 @@ public void testAuthoringPatterns() throws IOException { assertThat(distinct.size(), is(2)); - final String first = "Identifier hiding detected: Identifier for identifiers: [Diabetes] resolved as an alias of a query with exact case matching.\n"; - final String second = "Identifier hiding detected: Identifier for identifiers: [Application of Intermittent Pneumatic Compression Devices (IPC)] resolved as a value set with case insensitive matching.\n"; + final String first = "An alias identifier [Diabetes] is hiding another identifier of the same name. \n"; + final String second = "Are you sure you mean to use a valueset identifier [Application of Intermittent Pneumatic Compression Devices (IPC)], instead of [Application of intermittent pneumatic compression devices (IPC)]? \n"; assertThat(distinct, containsInAnyOrder(first, second)); }