[Feat] Add Advanced Security Properties to Repository Get/Update

SlyckLizzie commented 3 months ago

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

SlyckLizzie commented 3 months ago

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.

kfcampbell commented 3 months ago

@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.

nickfloyd commented 2 months ago

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!

SlyckLizzie commented 2 months ago

@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

SlyckLizzie commented 1 month ago

@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 commented 1 month ago

@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.

SlyckLizzie commented 1 month ago

@nickfloyd - It's all good. I'll make those changes by the end of the weekend. :)

SlyckLizzie commented 1 month ago

@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 commented 1 month ago

@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.

octokit / octokit.net