search

AngusJohnson / Clipper2

Polygon Clipping and Offsetting - C++, C# and Delphi
Boost Software License 1.0
1.52k stars 273 forks source link

c# code modernization #904

Closed angelofb closed 1 month ago

AngusJohnson commented 1 month ago

Wow, massive changes. Thanks! And from my very brief glimpse they all look fine (and certainly an improvement). However where my C# knowledge is limited, I'm probably not the best person to critique, so I'll defer merging to allow others with significant C# experience the opportunity to give feedback.

angelofb commented 1 month ago

most of it has been done with visual studio 2022 refactorings + roslynator. I have used many modern c# constructors like file scoped namespaces, primary constructors and collection initializers so the code is more tidy but no more copypastable in older projects.

LarsSkiba commented 1 month ago

Could you comment if your changes effect the minimal supported c# language version? That could be an interesting aspect. It's always nice to use improved language features but backwards compatibility might also be important for existing users.

angelofb commented 1 month ago

Clipper2Lib TargetFramework is still netstandard2.0 and I did not use constructs not compatible with older language standard in the public api so using the library with old frameworks should not be an issue.

JimBobSquarePants commented 1 month ago

image

However where my C# knowledge is limited, I'm probably not the best person to critique, so I'll defer merging to allow others with significant C# experience the opportunity to give feedback.

@AngusJohnson Honestly... As someone who maintains several complex libraries written in C# I would recommend against many of these changes and certainly not accept anything of this magnitude in a single PR. In my opinion readability has been seriously harmed with a focus on preferred style over safety.

angelofb commented 1 month ago

your libraries are awesome! just out of curiosity and can you point me where you think safety is harmed by these changes. If you are interested I can delete this PR and split into more fine grained PRs, guidance is welcomed.

AngusJohnson commented 1 month ago

@JimBobSquarePants, thank you for your helpful feedback. @angelofb, thank you again for your proposed changes but I'll heed advice and not merge your PR.