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.

Sign up for GitHub

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

Jump to bottom

Full implementation of detection of identifier hiding #1277

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Full implementation of detection of identifier hiding #1277

merged 5 commits into from
Nov 15, 2023

Conversation

lukedegruchy
Copy link
Contributor

@lukedegruchy lukedegruchy commented Nov 9, 2023 •
edited
Loading

  • Detect identifier hiding for various structures, including expressions, functions, parameters, arguments, lets, aliases, etc
  • Detect such hiding for both exact matches and case insensitive matches
  • Output compiler warnings referencing the structure in question
  • Ensure the context of the structure is considered. For example, remove from consideration an alias once the function body has been compiled
  • Tons of new tests and some updated tests.

Closes #426
Closes #721
Closes #1254

@lukedegruchy
Full implementation of detection of identifier hiding, including exac…
b1b2854 
…t and case-insensitive matches as well as a lot of new unit tests.
@lukedegruchy lukedegruchy marked this pull request as ready for review November 9, 2023 20:16
brynrhodes
brynrhodes requested changes Nov 10, 2023
View reviewed changes
Copy link
Member

@brynrhodes brynrhodes left a comment

Choose a reason for hiding this comment

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

Overall this looks really good, and I love all the tests, I just have some comments inline that I wonder about.

Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java Outdated Show resolved Hide resolved
Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java Outdated Show resolved Hide resolved
Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java Outdated Show resolved Hide resolved
Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java Outdated Show resolved Hide resolved
@JPercival JPercival enabled auto-merge (squash) November 10, 2023 17:47
@lukedegruchy lukedegruchy mentioned this pull request Nov 10, 2023
Closed
@JPercival JPercival requested a review from brynrhodes November 13, 2023 22:27
@JPercival
Copy link
Contributor

In review we split out a couple of additional stories here:
#598
#1280

brynrhodes
brynrhodes approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@brynrhodes brynrhodes left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@JPercival JPercival merged commit 9d92e5b into master Nov 15, 2023
2 checks passed
@JPercival JPercival deleted the 598-hiding-identifier-warn-exact-case-insensitive branch November 15, 2023 15:25
@JPercival JPercival mentioned this pull request Nov 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JPercival JPercival JPercival approved these changes

@brynrhodes brynrhodes brynrhodes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@lukedegruchy @JPercival @brynrhodes