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.

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

Validate Qualifiers at compile time #645

Merged
merged 11 commits into from
Jul 15, 2024

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented Jul 14, 2024

It seems previously we only validated that all the bean types were in the scope at compile time without looking at the qualifiers.

  • Change generated code to append qualifiers to dependency metadata
  • now will confirm that all qualified beans are in the scope when compiling

resolves #644

@SentryMan SentryMan added bug Something isn't working enhancement New feature or request labels Jul 14, 2024
@SentryMan SentryMan self-assigned this Jul 14, 2024
@SentryMan SentryMan requested a review from rbygrave July 14, 2024 03:34
@rob-bygrave
Copy link
Contributor

We should try to keep a PR focused on it's change only and not include other unrelated changes into it. My instinct is that "fix event publishers not getting detected" and "refactor reading external modules" should have been in separate PRs?

One aspect of this is that the PR Title is important documentation, and the other is from a git history perspective we want a commit to be focused on it's change. Putting multiple things into a commit makes life harder for our future selves when we try to understand the interaction between features by looking at commit history.

@SentryMan
Copy link
Collaborator Author

SentryMan commented Jul 15, 2024

My instinct is that "fix event publishers not getting detected" and "refactor reading external modules" should have been in separate PRs?

sure I can break the external one into it's own thing, but the event publisher bug only arises from the changes in this PR. I'm totally unable to replicate it when I try it on master.

@rob-bygrave
Copy link
Contributor

but the event publisher bug only arises from the changes in this PR

Ok cool. Then lets keep that in as part of this PR.

@SentryMan SentryMan added this to the 10.1 milestone Jul 15, 2024
@rbygrave rbygrave merged commit 116cbe4 into avaje:master Jul 15, 2024
7 checks passed
@SentryMan SentryMan deleted the validate-qualifiers branch July 16, 2024 00:16
rbygrave added a commit that referenced this pull request Jul 25, 2024
Fixes a regression that breaks partial compilation when qualifiers are used.

Regression introduced in 10.1 via:
https://github.com/avaje/avaje-inject/pull/645/files

With this regression we observe compile errors stating that dependencies with qualifiers are missing. When reviewing the relevant generated @metadata dependsOn attributes in the generate source we see that there are qualifiers incorrectly appended to the factory type.

The fix is in the MetaData constructor, such that the dependsOn does NOT use the name/qualifier of the type.
rbygrave added a commit that referenced this pull request Jul 25, 2024
* Fix partial compile regression introduced in 10.1 via #645

Fixes a regression that breaks partial compilation when qualifiers are used.

Regression introduced in 10.1 via:
https://github.com/avaje/avaje-inject/pull/645/files

With this regression we observe compile errors stating that dependencies with qualifiers are missing. When reviewing the relevant generated @metadata dependsOn attributes in the generate source we see that there are qualifiers incorrectly appended to the factory type.

The fix is in the MetaData constructor, such that the dependsOn does NOT use the name/qualifier of the type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
3 participants