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

DotNetCore -> DotNet Rename Missed Objects #3722

Closed
2 tasks done
BlythMeister opened this issue Nov 29, 2021 · 7 comments · Fixed by #3992
Closed
2 tasks done

DotNetCore -> DotNet Rename Missed Objects #3722

BlythMeister opened this issue Nov 29, 2021 · 7 comments · Fixed by #3992
Assignees
Milestone

Comments

@BlythMeister
Copy link
Contributor

Prerequisites

  • I have written a descriptive issue title
  • I have searched issues to ensure it has not already been reported

Cake runner

Cake .NET Tool, Cake Frosting, Cake runner for .NET Framework, Cake runner for .NET Core

Cake version

2.0.0-rcx

Operating system

N/A

Operating system architecture

N/A

CI Server

No response

What are you seeing?

The new DotNetMSBuildSettings object (https://github.com/cake-build/cake/blob/develop/src/Cake.Common/Tools/DotNet/MSBuild/DotNetMSBuildSettings.cs) iteself references some DotNetCore namespaced classes.

What is expected?

DotNetCore namespace is all obsolete and replaced with the DotNet namespace

Steps to Reproduce

See file https://github.com/cake-build/cake/blob/develop/src/Cake.Common/Tools/DotNet/MSBuild/DotNetMSBuildSettings.cs it has a using statement Cake.Common.Tools.DotNetCore.MSBuild; as it neededs things in the DotNetCore namespace.

Output log

No response

@augustoproiete
Copy link
Member

Thanks @BlythMeister. This one you spotted is "by design".

The goal was to introduce the new DotNet*** aliases without introducing breaking changes to existing DotNetCore*** aliases.

Unfortunately the class MSBuildLoggerSettings which is in the old ...DotNetCore namespace doesn't have the Core suffix on it, so I couldn't add it to the new ...DotNet namespace and keep the old one at the same time as I did with all the other classes, because that causes ambiguity in Cake scripts as the compiler doesn't know which one to pick...

The plan is to remove all the DotNetCore*** on Cake 3.0 ➡️ #3341 at which point the old ...DotNetCore namespace will disapear.

That said, the fact that MSBuildLoggerSettings relies on the old namespace (in theory) should be an implementation detail and should not make any difference for the user in Cake scripts and with most Frosting builds...

The only scenario I can think of where you would still need to include the old namespace is Frosting-only, if you specifically use MSBuildLoggerSettings in your code.

Let me know if I missed anything

@BlythMeister
Copy link
Contributor Author

Yeah, that's exactly how I noticed it, in my frosting project I bulk removed the "Core" from "DotNetCore" and this updated the using.

It's not something I'm too concerned about to be honest and the reasoning makes perfect sense.

So this can be closed 👍

@BlythMeister
Copy link
Contributor Author

The only other potential idea could be to move the classes in cake 2.x.
And make the "DotNetCore" namespaced versions reference the objects in the new namespace?

At some point, this will be a hard break for someone using frosting whereby the namespace is in a using statement.

Is it better to make that hard break now so its when they are changing to the new alias?

@augustoproiete
Copy link
Member

augustoproiete commented Nov 30, 2021

Is this what you mean?

1.) Copy Move Cake.Common.Tools.DotNetCore.MSBuild.MSBuildLoggerSettings to Cake.Common.Tools.DotNet.MSBuild.MSBuildLoggerSettings

2.) Rename Cake.Common.Tools.DotNetCore.MSBuild.MSBuildLoggerSettings to something like Cake.Common.Tools.DotNetCore.MSBuild.MSBuildCoreLoggerSettings Renaming doesn't help... Might as well just move on step 1

?

Therefore breaking Frosting now, rather than in 3.0?

@BlythMeister
Copy link
Contributor Author

Yeah exactly (and any of the other classes in the DotNetCore namespace which don't have DotNetCore in the name.

I think it will mean 1 breaking release rather than 2.
For me, obsolete is breaking as our corporate environment has warnings as errors enabled 😂

@augustoproiete
Copy link
Member

@BlythMeister Unfortunately we missed the boat on this one... Cake v2.0 shipped earlier today, so we'll have to schedule this one for v3.0

@augustoproiete augustoproiete added this to the v3.0.0 milestone Feb 6, 2022
@augustoproiete augustoproiete changed the title [2.0.0rc] DotNetCore -> DotNet Rename Missed Objects DotNetCore -> DotNet Rename Missed Objects Feb 6, 2022
@devlead devlead self-assigned this Oct 17, 2022
devlead added a commit to devlead/cake that referenced this issue Oct 17, 2022
devlead added a commit to devlead/cake that referenced this issue Oct 17, 2022
devlead added a commit that referenced this issue Oct 18, 2022
GH3722/GH3991: Remove Obsolete DotNetCore aliases
@cake-build-bot
Copy link

🎉 This issue has been resolved in version v3.0.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

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

Successfully merging a pull request may close this issue.

4 participants