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

Target to net 7 and 8 in addition to 6 #419

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

virzak
Copy link
Contributor

@virzak virzak commented Feb 24, 2023

Description:

Fixes # (issue)

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested my changes by running the extension in VS2017
  • I have tested my changes by running the extension in VS2019
  • I have tested my changes by running the extension in VS2022
  • If changes to the documentation are needed, I have noted this in the description above

@virzak virzak marked this pull request as ready for review March 7, 2023 00:44
@grochocki
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@grochocki grochocki left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! A couple questions, but this otherwise generally looks good.

@hansmbakker
Copy link

Given that .NET 7 goes out of support on May 14, 2024, I'd suggest adding only .NET 8 instead of 7 and 8.

Other than that, great that you're contributing this!

@hansmbakker
Copy link

hansmbakker commented Jan 25, 2024

@grochocki having .NET 8 support would really be a welcome update, so that users are not forced to install .NET 6 to run the XamlStyler CLI.

Would it be an idea to do the targetframework update to .NET 8 independent of the other changes in this PR (dependency to Newtonsoft.Json, change in VS SDK version, allowPrerelease in global.json), to unblock this?

(In the meantime, I can add a .NET 8 version of the CLI to my private nuget feed as a workaround)

@@ -0,0 +1,5 @@
{
"sdk": {
"allowPrerelease": true

Choose a reason for hiding this comment

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

is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes no difference for .NET 8, but when .NET 9 comes out, we'll need this. Some people including myself switch to preview early, so I vote to leave this in.

build/pipelines/templates/build-console-public.yaml Outdated Show resolved Hide resolved
@heartacker
Copy link

hi,all,can any body tell that what block this, thanks

@MarkPflug
Copy link

Instead of multi-targeting, I'd suggest adding a <RollForward>Major to the xstyle .csproj. This would allow you to continue to build against a single framework version (6.0), but allow the tool to run on future versions without needing to be updated.

https://learn.microsoft.com/en-us/dotnet/core/versions/selection#control-roll-forward-behavior

@nietras
Copy link

nietras commented Jul 11, 2024

We have seen the same issue with XamlStyler and think it would be great if RollForward Major was supported as @MarkPflug mentions.

@virzak
Copy link
Contributor Author

virzak commented Jul 17, 2024

Instead of multi-targeting, I'd suggest adding a <RollForward>Major to the xstyle .csproj. This would allow you to continue to build against a single framework version (6.0), but allow the tool to run on future versions without needing to be updated.

https://learn.microsoft.com/en-us/dotnet/core/versions/selection#control-roll-forward-behavior

@MarkPflug That works too. @grochocki what's your opinion?

@heartacker
Copy link

anyupdate?

@Jeanjean
Copy link

What can we do to get this MR completed?

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.

7 participants