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

Expose API for imported projects checks #10761

Merged
merged 12 commits into from
Oct 19, 2024
Merged

Expose API for imported projects checks #10761

merged 12 commits into from
Oct 19, 2024

Conversation

YuliiaKovalova
Copy link
Member

@YuliiaKovalova YuliiaKovalova commented Oct 7, 2024 •
edited
Loading

Related to #10605

In this PR, the following changes are implemented:

Expose API for writing custom checks for imported projects.
Make CheckScopeClassifier functionality public.
Publish ElementLocation.Create method to be used outside of MSBuild.
Fix issue in custom check template (embedding 3rd party references in the package).

@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/custom_check branch from 67febab to d8c8b69 Compare October 7, 2024 11:04
maridematte
maridematte reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@maridematte maridematte left a comment

Choose a reason for hiding this comment

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

This PR has a few changes that I'd prefer to have their own PR (even if it is small). I'd also like to see a more robust PR description outlining why we are doing these changes.

src/Build/ElementLocation/ElementLocation.cs Show resolved Hide resolved
JanKrivanek
JanKrivanek reviewed Oct 7, 2024
View reviewed changes
Copy link
Member

@JanKrivanek JanKrivanek 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!

I'm not signing of yet - as I want to discuss imports ordering and ability to store deffered data bit more conscise by indexing by just EvalId.

If you feel strongly opinionated - feel free to respond and re-request the review ;-)

src/Build/ElementLocation/ElementLocation.cs Show resolved Hide resolved
src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova mentioned this pull request Oct 8, 2024
Open
JanKrivanek
JanKrivanek requested changes Oct 14, 2024
View reviewed changes
Copy link
Member

@JanKrivanek JanKrivanek 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!

The only minor thing I have - I just want clear public API naming + comments. Once that's done - this feels ready to go!

src/Build/BuildCheck/OM/ProjectImportedCheckData.cs Outdated Show resolved Hide resolved
src/Build/BuildCheck/OM/ProjectImportedCheckData.cs Show resolved Hide resolved
src/Build/BuildCheck/OM/ProjectImportedCheckData.cs Outdated Show resolved Hide resolved
JanKrivanek
JanKrivanek approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!!

I added one more suggestion - not blocking though.

@YuliiaKovalova YuliiaKovalova merged commit ea75a75 into dotnet:main Oct 19, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JanKrivanek JanKrivanek JanKrivanek approved these changes

@maridematte maridematte maridematte approved these changes

@baronfel baronfel Awaiting requested review from baronfel

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

Successfully merging this pull request may close these issues.
3 participants
@YuliiaKovalova @JanKrivanek @maridematte