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

add DotnetFramework and DotnetCore features #87

Closed
wants to merge 5 commits into from

Conversation

jonlouie
Copy link
Contributor

@jonlouie jonlouie commented Mar 24, 2021

Related issue

Closes: #83

Description

  • Detects if a project is .NET Framework, .NET Core, or .NET Standard
  • Looks at .csproj files and uses regex patterns to identify the target framework type
  • Since regex patterns are used, the project type is identified based on intent of the developer, not based on correctness (e.g. netcoreapp.3.9 is an invalid target framework, but it would still register as a .NET Core project)

Supplemental testing

Describe any testing done in addition to existing unit tests

Additional context

Sources of Target Framework patterns


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@Eruanion Eruanion left a comment

Choose a reason for hiding this comment

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

Awesome code, just that one comment based on empty document possible problem.

@Eruanion Eruanion self-requested a review March 24, 2021 17:57
Eruanion
Eruanion previously approved these changes Mar 24, 2021
Copy link
Contributor

@marknfawaz marknfawaz left a comment

Choose a reason for hiding this comment

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

One more condition to consider is the global.json file that sets the version for all projects within a directory:

https://docs.microsoft.com/en-us/dotnet/core/tools/global-json?tabs=netcore3x

Comment on lines 52 to 53
?? _csproj.GetElementsByLocalName(Constants.TargetFrameworkElement).FirstOrDefault()
?? _csproj.GetElementsByLocalName(Constants.TargetFrameworksElement).FirstOrDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can both TargetFramework and TargetFrameworks exist at the same time? If yes, this will not give a correct result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is technically valid for both TargetFramework and TargetFrameworks to exist in a .csproj file, but in those cases, TargetFrameworks will be ignored (source)

@jonlouie
Copy link
Contributor Author

One more condition to consider is the global.json file that sets the version for all projects within a directory:

https://docs.microsoft.com/en-us/dotnet/core/tools/global-json?tabs=netcore3x

@marknfawaz This is an interesting case. At first glance, it doesn't seem like global.json is coupled to a project and only customizes the CLI execution environment (as opposed to declaring the target framework inherent to the project). I don't think this is a case we'll need to factor in here, but I could be wrong. If my understanding is incorrect, we can talk discuss more in detail.

@jonlouie
Copy link
Contributor Author

One more condition to consider is the global.json file that sets the version for all projects within a directory:
https://docs.microsoft.com/en-us/dotnet/core/tools/global-json?tabs=netcore3x

@marknfawaz This is an interesting case. At first glance, it doesn't seem like global.json is coupled to a project and only customizes the CLI execution environment (as opposed to declaring the target framework inherent to the project). I don't think this is a case we'll need to factor in here, but I could be wrong. If my understanding is incorrect, we can talk discuss more in detail.

Per our conversation, I will update the features to use the build result to get the target framework(s).

@jonlouie
Copy link
Contributor Author

Closing- obsolete

@jonlouie jonlouie closed this Feb 13, 2023
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.

Add feature to detect if a project is a core project
4 participants