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

Be consistent about ifdefs for .NET 5 #22194

Merged
merged 2 commits into from
Aug 24, 2020
Merged

Be consistent about ifdefs for .NET 5 #22194

merged 2 commits into from
Aug 24, 2020

Conversation

ajcvickers
Copy link
Member

No description provided.

@ajcvickers ajcvickers requested a review from a team August 24, 2020 14:46
@roji
Copy link
Member

roji commented Aug 24, 2020

Shouldn't there be the one standard way to test for this, rather than all three options? I'm guessing NET50 is supposed to be that, if it works should just use it?

@ajcvickers
Copy link
Member Author

@roji At this point I would rather be safe and consistent than minimal.

@bricelam
Copy link
Contributor

It should be NET5_0--the SDK unifies to this.

@ajcvickers
Copy link
Member Author

@bricelam That's the one that doesn't work universally.

@bricelam
Copy link
Contributor

In other words net50, net5.0, and netcoreapp5.0 all result in the NET5_0 define constant

@bricelam
Copy link
Contributor

What SDK do you have installed? I'm seeing this in preview.7

@ajcvickers
Copy link
Member Author

@bricelam I'm using the one installed by our build--which I believe is preview 7 currently. However, it may be that my IDE doesn't understand it yet.

@bricelam
Copy link
Contributor

Does NET5_0 || NETCOREAPP5_0 work? I'm good with that.

@ajcvickers
Copy link
Member Author

Also, NET5_0 is soooo ugly!

@bricelam
Copy link
Contributor

Could just use NET...

@bricelam
Copy link
Contributor

Oh wait, no we can't. 🤦‍♂️

@ajcvickers
Copy link
Member Author

Okay, updated to just NET5_0. This builds for me in my IDE, although there are some syntax highlighting issues.

@ajcvickers
Copy link
Member Author

@dotnet/efteam Approve?

@ajcvickers ajcvickers merged commit d568a93 into release/5.0 Aug 24, 2020
@ajcvickers ajcvickers deleted the BeConst0824 branch August 24, 2020 18:53
@ajcvickers ajcvickers mentioned this pull request Aug 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smitpatel smitpatel smitpatel approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@ajcvickers @roji @bricelam @smitpatel