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

dotnet: Update to .NET 8 and most recent protobuf #131

Merged
merged 7 commits into from
Jul 1, 2024

Conversation

HopXXII
Copy link
Contributor

@HopXXII HopXXII commented Jan 8, 2024

Created .NET 8 version which is the most recent version of .NET Core (#56) and a LTS successor of .NET 6 (#127).

@jameslinjl
Copy link
Contributor

@bdferris-v2 Are you able to take a look at this? I haven't had any bandwidth to work on GTFS stuff lately :(

@bdferris-v2
Copy link
Collaborator

Two questions:

  1. Your PR seemingly shows diffs for many of the text files (README.md, UPDATING.md, .gitignore) where the entire file is marked as a diff, but the contents don't look different. This is making it hard to tell if there were any small content changes to these files. Did something like the line-ending change on these files? Any chance you could clean this up?

  2. It's been a long time I've done anything with .NET, so forgive the dumb question. Is there any downside to targeting .NET 8 as opposed to .NET 6? I understand that 8 is the LTS successor for 6, but will upgrading to 8 now limit applications that want to use this library that are still on 6?

@EnessenE
Copy link

EnessenE commented Mar 3, 2024

I am a bit late in this discussion, but I would like to butt in.

I would recommend targetting neither .NET 6 or .NET 8 but targetting .NET Standard 2.0 (or .1) instead. Targetting this gives the widest available range of compatibility as seen here. https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0

Targetting specific versions should only be done if you are going for a feature that is only available in that specific target framework.

@EnessenE
Copy link

EnessenE commented Mar 3, 2024

I ended up doing that one for comparison. Converted to .NET standard 2.0 and the new "SDK" style for .csproj's. Kept is as a draft to not overshadow this PR (didnt upgrade protobuf for example)

@scottpidzarko
Copy link

scottpidzarko commented May 31, 2024

I like @EnessenE's draft PR - it's simple and to the point, and having it be a net standard library just makes sense for a library of this purpose. It's been nearly 3 months without any discussion about this and I would love to have a brand new application I am working on be .NET 8 instead of .NET (framework) 4.8.

What would be holding back bringing this PR for net standard 2.0 out of draft, rejecting the other one that has the aforementioned issues, and approving the PR for net standard 2.0?

@HopXXII
Copy link
Contributor Author

HopXXII commented Jun 26, 2024

First, I apologize for my long inactivity.
To answer @bdferris-v2:

  1. Sure, silly mistake, I'll fix it.
  2. It makes more sense to me to make the .NET Standard version, as others write. I didn't think of it before, but compatibility will be the greatest there, and that's actually what we want.

I'll try to work it in by the end of the week.

@HopXXII
Copy link
Contributor Author

HopXXII commented Jul 1, 2024

I think it's ready for review, @bdferris-v2.

@bdferris-v2 bdferris-v2 merged commit 0093fd0 into MobilityData:master Jul 1, 2024
@scottpidzarko
Copy link

Thanks for the motion on this, I really appreciate it :)

@EnessenE
Copy link

Small afterwards note: would be nice if someone pushed this to nuget ;)

@HopXXII
Copy link
Contributor Author

HopXXII commented Aug 11, 2024

Small afterwards note: would be nice if someone pushed this to nuget ;)

@bdferris-v2 is one of the owners in the NuGet gallery :)

@scottpidzarko
Copy link

@bdferris-v2 any chance a NuGet package can be created which encapsulates this change? We've made a private Nuget package for our internal use with .net 8 but I'd like to use the public version when possible.

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.

5 participants