octokit.net
octokit.net copied to clipboard
octokit
[Feat] Add Advanced Security Properties to Repository Get/Update
Resolves #ISSUE_NUMBER N/A
Before the change?
- Repository get/update did not implement Advanced Security properties
After the change?
- Repository get/update implements Advanced Security properties
Pull request checklist
- [ ] Tests for the changes have been added (for bug fixes / features)
- [x] Docs have been reviewed and added / updated if needed (for bug fixes / features)
Does this introduce a breaking change?
Please see our docs on breaking changes to help!
- [x] Yes
- [ ] No
Breaking change is because the current C# language is targeting 7.3 and in order to make a reference object nullable it needs to be targeting at least 8.0. The repository response and update objects will require implementers to add in the SecurityAndAnalysis objects.
@nickfloyd are you comfortable with this support bump? C# 7.0 came out in 2017, but I can't seem to find any information on the support windows. Some versions of .NET still default to C# 7.x, though I'm not sure if we should still support those.
Breaking change is because the current C# language is targeting 7.3 and in order to make a reference object nullable it needs to be targeting at least 8.0. The repository response and update objects will require implementers to add in the SecurityAndAnalysis objects.
Hey @SlyckLizzie, for clarity reference types in c# have always been nullable. I am guessing you are referring to the enum that was added - status, is that correct?
Enums, which are value types, will allow the nullable operator (since it was introduced in c# 2.0) the only real difference is that we have to do some safety checks like .Value, .HasValue, or string comparison checks.
If you get the chance, would you let me know where in your change set is the modification that will require .NET 8.0? I am sure I am missing it - perhaps with the way we serialize and deserialize things? Let me know so that I can better understand the constraint.
Somewhat related we still have users on this SDK that are still on C# 7.x (LTS) - which is still in maintenance until May 14, 2024 and .NET 6 which is active until November 12, 2024 - we tend to follow the language release cycle for this SDK.
FYI, our generative .NET SDK is targeting .NET 8 as is.
Thanks for this addition, I'm looking forward to getting it in!
@nickfloyd - It's not the framework version but the language version targeted by framework in the project file. netstandard2.0 targets the c# language 7.3. In order for me to be able to make the SecurityAndAnalysisRequest input parameter of the Respository Response model nullable, e.g. SecurityAndAnalysisRequest? the target framework of the project needs to be netstandard2.1 to target the c# language version 8.0
https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version
@nickfloyd - Is there still an issue with this? I noticed a new breaking change version update to the package. Why not add this in with that?
@nickfloyd - Is there still an issue with this? I noticed a new breaking change version update to the package. Why not add this in with that?
Because I completely dropped the ball on this. I'm sorry about that...
We can move forward with what you've suggested. I'll need to roll it out in a new major release (that's on me) but it should be fine. Go ahead and change the target to 2.1 when you get the chance and we can get this out the door.
Also, would you mind bumping Octokit.Reactive as well?
Again, apologies for the delay.
@nickfloyd - It's all good. I'll make those changes by the end of the weekend. :)
@nickfloyd - Apparently that change isn't going to play nice with the .Net Framework. If it's not .net core or later then it has to reference net standard 2.0. The test and pagnationextension projects still have a reference to 4.62. Everything seems to compile fine without the 4.62 but I want to make sure that's the direction you want to go.
https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-1
@nickfloyd - Apparently that change isn't going to play nice with the .Net Framework. If it's not .net core or later then it has to reference net standard 2.0. The test and pagnationextension projects still have a reference to 4.62. Everything seems to compile fine without the 4.62 but I want to make sure that's the direction you want to go.
I was really hoping that all of the projects were going to play well with this change. Let me see if I can determine if 4.6.2 is actually a requirement in those projects or if they can be bumped up as well.
I still believe moving this direction is the right way to go - but it might take a bit more work to get there. Thanks again for the heads up... I'll have a look and let you know what I find / or if you beat me to it - either way.
@nickfloyd any status update on this. I'd really like to see it get included in the next release if possible
@nickfloyd any status update on this. I'd really like to see it get included in the next release if possible
Hey @ddaniels-andmore I am planning on taking a look this Friday (06/07). I'll comment and let you know what I find/come up with.
@nickfloyd - What's blocking this from being added as part of a breaking change release?
Hey @SlyckLizzie the quick answer is this. The issue began breaking several implementations now that integers have grown past the original int32 size - that ended up consuming our time because we had to correct some things with our OpenAPI definitions and in the new generated SDKs. I'm going to see if I can combine this change with all of the breaking changes from the Int32 to Int64 shift and roll all of it out this week.
Apologies for the delay. Given that our team is so small we sometimes face challenges with prioritization that slow things down quite a bit. This is one of the reasons @kfcampbell and I are so grateful for a community with people like you that move things forward even when we cannot. ❤️