Skip to content

Commit

Permalink
More code review suggestions.
Browse files Browse the repository at this point in the history
  • Loading branch information
@lukedegruchy
lukedegruchy committed Nov 10, 2023
1 parent ea44f8a commit 416ef86
Show file tree
Hide file tree
Showing 6 changed files with 54 additions and 92 deletions.
72 changes: 27 additions & 45 deletions Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]";
}

/**
Expand Down Expand Up @@ -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);
}
}
Expand Down
17 changes: 0 additions & 17 deletions Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java
View file

This file was deleted.

27 changes: 12 additions & 15 deletions Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public void testCaseInsensitiveWarning() throws IOException {
final List<CqlCompilerException> warnings = translator.getWarnings();
assertThat(warnings.toString(), translator.getWarnings().size(), is(1));
final Set<String> 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
Expand All @@ -28,7 +28,7 @@ public void testHiddenIdentifierFromReturn() throws IOException {

assertThat(warnings.toString(), translator.getWarnings().size(), is(1));
final Set<String> 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
Expand All @@ -49,10 +49,7 @@ public void testHidingUnionWithSameAliasEachHides() throws IOException {
final List<String> 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
Expand All @@ -69,7 +66,7 @@ public void testSoMuchNestingHidingSimple() throws IOException {
final List<CqlCompilerException> 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
Expand All @@ -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));
}
Expand All @@ -97,7 +94,7 @@ public void testHidingLetAlias() throws IOException {

final List<String> 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
Expand All @@ -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
Expand All @@ -124,7 +121,7 @@ public void testHidingFunctionDefinitionWithOverloads() throws IOException {
final List<CqlCompilerException> warnings = translator.getWarnings();
final List<String> 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
Expand All @@ -133,7 +130,7 @@ public void testHidingParameterDefinition() throws IOException {
final List<CqlCompilerException> warnings = translator.getWarnings();
final List<String> 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
Expand All @@ -142,7 +139,7 @@ public void testHidingIncludeDefinition() throws IOException {
final List<CqlCompilerException> warnings = translator.getWarnings();
final List<String> 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
Expand All @@ -153,6 +150,6 @@ public void testHidingCommaMissingInListConstruction() throws IOException {
assertThat(warningMessages.toString(), warnings.size(), is(2));
final List<String> 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"));
}
}
22 changes: 11 additions & 11 deletions Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
4 changes: 2 additions & 2 deletions Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
4 changes: 2 additions & 2 deletions Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down

0 comments on commit 416ef86

Please sign in to comment.