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

Do not silently fail in case of class scanning exceptions #98

Merged
merged 3 commits into from
Feb 3, 2024
Merged

Do not silently fail in case of class scanning exceptions #98

merged 3 commits into from
Feb 3, 2024

Conversation

kwin
Copy link
Contributor

@kwin kwin commented Oct 17, 2023

This closes #97

@kwin kwin requested review from cstamas and mcculls October 17, 2023 17:44
@kwin
Copy link
Contributor Author

kwin commented Oct 17, 2023

@cstamas Any idea how to make this configurable from Maven? Do we really want to extend the Plexus Container API for that?

kwin
kwin commented Oct 17, 2023
View reviewed changes
org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/space/SisuIndex.java
@@ -92,7 +92,7 @@ public final void index( final ClassSpace _space )
{
try
{
new SpaceScanner( _space ).accept( this );
new SpaceScanner( _space, true ).accept( this );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess being always strict here should be fine

kwin
kwin commented Oct 17, 2023
View reviewed changes
org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/space/SpaceModule.java Outdated
@@ -166,7 +166,7 @@ public SpaceVisitor visitor( final Binder binder )

void scanForElements( final Binder binder )
{
new SpaceScanner( space, finder ).accept( strategy.visitor( binder ) );
new SpaceScanner( space, finder, true ).accept( strategy.visitor( binder ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should probably expose a way on how to toggle the strict flag.

@kwin kwin force-pushed the feature/strict-space-scanner branch from 6b23ed9 to f45f781 Compare October 17, 2023 17:48
mcculls
mcculls requested changes Oct 22, 2023
View reviewed changes
Copy link
Contributor

@mcculls mcculls left a comment

Choose a reason for hiding this comment

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

I think the default mode should remain non-strict, for reasons discussed in #97 (comment)

We can still change the mode to strict when generating the index via annotation processing or the maven plugin. That should solve the main build-time issue without affecting runtime deployments.

@kwin
Copy link
Contributor Author

kwin commented Oct 24, 2023

@mcculls Can you be more specific, where exactly you want to keep the default behaviour of being lenient?

  1. https://github.com/eclipse/sisu.inject/blob/d2a7e64ad07a026ad488b042a37744e66ff9a75e/org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/space/SisuIndex.java#L95
  2. https://github.com/eclipse/sisu.inject/blob/d2a7e64ad07a026ad488b042a37744e66ff9a75e/org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/space/SpaceModule.java#L169
  3. https://github.com/eclipse/sisu.inject/blob/d2a7e64ad07a026ad488b042a37744e66ff9a75e/org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/space/QualifierCache.java#L83

or anywhere else (which I missed in the list above)

@kwin
Copy link
Contributor Author

kwin commented Nov 12, 2023

@mcculls Can you comment on the individual cases listed above?

@mcculls
Copy link
Contributor

mcculls commented Jan 7, 2024

SisuIndex can be strict by default because it's build-time - everything else should be non-strict by default, with an option to make them strict.

@kwin kwin force-pushed the feature/strict-space-scanner branch from c6b97c6 to 7bc2ae3 Compare January 8, 2024 13:14
@kwin kwin marked this pull request as ready for review January 8, 2024 13:17
@kwin
Copy link
Contributor Author

kwin commented Jan 8, 2024

everything else should be non-strict by default, with an option to make them strict.

@mcculls done now, can you check again?

@kwin kwin requested a review from mcculls January 8, 2024 13:17
@kwin
Copy link
Contributor Author

kwin commented Jan 30, 2024

If there are no further concerns @mcculls I am gonna merge end of this week.

@kwin kwin mentioned this pull request Jan 31, 2024
Closed
@kriegaex kriegaex mentioned this pull request Feb 1, 2024
Closed
@kwin kwin merged commit 842994b into master Feb 3, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cstamas cstamas Awaiting requested review from cstamas

@mcculls mcculls Awaiting requested review from mcculls

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do not silently fail in SpaceScanner when classes could not be parsed
2 participants
@kwin @mcculls