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

C#: Update DependencyManager logic to exclude commented out references. #14665

Merged
merged 3 commits into from Nov 3, 2023

Conversation

michaelnebel
Copy link
Contributor

In the DependencyManager we do some processing of eg. PackageReference and other content.
However, in some cases this might be commented out.
That is, the dependency manager shouldn't detect that we have a package dependency when it comes across something like

<!-- <PackageReference Include=\"NUnit\" Version=\"3.10.1\" /> -->

as this is commented out.

@github-actions github-actions bot added the C# label Nov 2, 2023
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Nov 2, 2023
@michaelnebel michaelnebel marked this pull request as ready for review November 2, 2023 17:43
@michaelnebel michaelnebel requested a review from a team as a code owner November 2, 2023 17:43
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

Have you seen commented entries in multiple projects? Or was this change motivated by our integration test?

@michaelnebel
Copy link
Contributor Author

michaelnebel commented Nov 3, 2023

Have you seen commented entries in multiple projects? Or was this change motivated by our integration test?

It was motivated by our integration tests, but it doesn't seem to be that uncommon (I am pretty sure I have done stuff like that in the past myself :-) ). Also searching on GitHub using path:*.csproj /<!--.*PackageReference/ gives more than 11k hits, so maybe it doesn't hurt to include this in general.
The other patterns are less likely though (FrameworkReference) and so on, but I added those for consistency.
We could consider to only make the negative look behind on the PackageReference matching. What do you think?

@tamasvajk
Copy link
Contributor

What do you think?

Let's keep them. If we see they don't work for some reason, we'll remove them.

@michaelnebel michaelnebel merged commit 4009f03 into github:main Nov 3, 2023
16 checks passed
@michaelnebel michaelnebel deleted the csharp/projectreference branch November 3, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants