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

Support Internal Types In AssemblyScanner #1742

Conversation

iabdelkareem
Copy link
Contributor

The commit solves #1741 by changing AssemblyScanner behavior to look for all types in an assembly instead of exported typed.

The commit solves FluentValidation#1741 by changing AssemblyScanner behavior to look for all types in an assembly instead of exported typed.
@dnfadmin
Copy link
Collaborator

dnfadmin commented May 18, 2021

CLA assistant check
All CLA requirements met.

@JeremySkinner
Copy link
Member

Hi @iabdelkareem please see my comment #1741 (comment)

This will need to be an opt-in behaviour. I'd suggest adding an additional optional parameter to the DI extension methods includeInternalTypes = false, which can then be passed through to the assembly scanner.

If you're able to make these changes I'll then get this reviewed and merged. Thanks.

- Added support to optionally include internal types in  AssemblyScanner and DependencyInjectionExtensions via includeInternalTypes optional parameter

- Added Test Coverage.

- Added documentation to address the change.

FluentValidation#1741
@iabdelkareem
Copy link
Contributor Author

Thank you for the quick response and explanation. I agree with you it's much better when including internal types is opt-in. I've updated the PR with the changes including test coverage and documentation.

@JeremySkinner
Copy link
Member

Thanks, I’m going to be busy for the next couple of days but I’ll try and review it over the weekend.

@JeremySkinner JeremySkinner merged commit d256435 into FluentValidation:main May 21, 2021
@JeremySkinner
Copy link
Member

Merged, thanks!

@JeremySkinner JeremySkinner added this to the 10.2 milestone May 21, 2021
@iabdelkareem
Copy link
Contributor Author

Thank you ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants