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

Upgrade to .NET 8 #191

Merged
merged 9 commits into from
Oct 25, 2023
Merged

Conversation

IEvangelist
Copy link
Member

@IEvangelist IEvangelist commented Oct 13, 2023

I just now saw #186, I didn't realize before that there was an effort in place to do this work. There are parts of that PR that look good—@luisquintanilla how should we proceed?

I pulled in the changes from the PR that I liked. Let's add the CPM in a separate PR, since versions weren't bumped.

I've bumped the versions, and I'm updating this PR to utilize the CPM feature since it does improve the PacakgeReference experience.

@luisquintanilla
Copy link
Collaborator

Thanks @IEvangelist

@luisquintanilla
Copy link
Collaborator

I just now saw #186, I didn't realize before that there was an effort in place to do this work. There are parts of that PR that look good—@luisquintanilla how should we proceed?

I pulled in the changes from the PR that I liked. Let's add the CPM in a separate PR, since versions weren't bumped.

I've bumped the versions, and I'm updating this PR to utilize the CPM feature since it does improve the PacakgeReference experience.

Sounds good @IEvangelist. Thanks.

Which parts from #186 are not in this PR?

@aaronpowell
Copy link
Member

Just be aware that #186 builds on top of #184 so there are some unrelated changes are the VS Code experience in the PR.

@aaronpowell
Copy link
Member

I noticed that this PR also upgrades to C# 12 preview - I'd suggest not doing that in this PR but in a follow-up one to ensure that it's easier to review the .NET 8 specific changes without also having language changes in there.

Small and targeted PRs are easier to review and remediate.

@luisquintanilla
Copy link
Collaborator

@LittleLittleCloud do these changes affect the App Insight / Authentication changes you've made?

@LittleLittleCloud
Copy link
Collaborator

@luisquintanilla Shouldn't be effected.

@luisquintanilla
Copy link
Collaborator

Cool. Let's merge those in first then and do a quick test and follow of these changes.

@aaronpowell
Copy link
Member

Shall we close #186 in favour of this?

Yesterday I noticed a problem in my PR with VS Code debugging which I addressed in 178aa84 (although there's another problem that I'm digging through)

@luisquintanilla
Copy link
Collaborator

Shall we close #186 in favour of this?

Yesterday I noticed a problem in my PR with VS Code debugging which I addressed in 178aa84 (although there's another problem that I'm digging through)

I'm okay with that. Should #184 be closed as well since #186 built on top of it?

@aaronpowell
Copy link
Member

Shall we close #186 in favour of this?
Yesterday I noticed a problem in my PR with VS Code debugging which I addressed in 178aa84 (although there's another problem that I'm digging through)

I'm okay with that. Should #184 be closed as well since #186 built on top of it?

I'd merge #184 first as the changes it introduces are unrelated to the .NET 8 upgrade, so it would give better history tracking of when/why changes were made.

@LittleLittleCloud LittleLittleCloud merged commit 092e1db into Azure-Samples:main Oct 25, 2023
4 checks passed
IEvangelist added a commit to IEvangelist/azure-search-openai-demo-csharp that referenced this pull request Dec 5, 2023
I just now saw Azure-Samples#186, I didn't realize before that there was an effort in
place to do this work. There are parts of that PR that look
good—@luisquintanilla how should we proceed?

I pulled in the changes from the PR that I liked. ~Let's add the CPM in
a separate PR, since versions weren't bumped~.

I've bumped the versions, and I'm updating this PR to utilize the CPM
feature since it does improve the `PacakgeReference` experience.

---------

Co-authored-by: XiaoYun Zhang <xiaoyuz@microsoft.com>
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.

4 participants