ironfede / openmcdf

Microsoft Compound File .net component - pure C# - netstandard 2.0
Mozilla Public License 2.0
314 stars 78 forks source link

Solution/Project update #156

Closed jeremy-visionaid closed 1 month ago

jeremy-visionaid commented 2 months ago

This does a few things:

I added a placeholder .globalconfig from one of my own projects for the analyzer configuration, but you should definitely check it out locally first to see how you'd like it handled (if at all, but it gives about 3.5k warnings up from 200 currently): https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-files

Basically there are 3 options:

  1. Apply the analyzer fixes
  2. Silence the warnings in a .globalconfig
  3. Just drop StyleCop for the PR

So, I'd need to update it based on your preferences - I would obviously try and be as sympathetic as possible to the existing style of the project. The main intent here is that by specifying the style and enforcing it, it's more likely that we'd be able to cherry-pick and/or merge fixes cleanly between 2.x and 3.x.

Just let me know if you'd like me to re-work or drop any of it 👍

ironfede commented 1 month ago

Thank you very much @jeremy-visionaid for your PRs. I'm reviewing all your changes. First of all, I'm getting a warning in package dependencies in VS2022 that I can't fully understand...

image

Do you have any idea about why this happens?

Anyhow, It's ok to me using StyleCop but , if You agree, not in 2.x branch.

I would avoid introducing new dependencies in a long existing release/branch of OpenMcdf to avoid a dependency graph change for current user base. It's absolutely a good addition but I'd prefer to introduce it in the 3.x branch where there will be more freedom to apply breaking changes or interface evolutions.

I would go to a total feature freeze in 2.x branch for a final 2.4 release supporting old .net framework.

3.x should targeting .net 8 exclusively imho.

Please let me know If this sounds good to you and to evrerybody with interests in OpenMcdf reading this comment.

jeremy-visionaid commented 1 month ago

@ironfede That's curious about the warning - I don't get it on mine. Maybe try a close, git clean -dfx and reopen, and see if it happens. I suspect that it's an environmental issue.

WRT. to introducing additional dependencies. the PackageReference has <PrivateAssets>all</PrivateAssets> to prevent StyleCop flowing through as a dependency to other projects. i.e. it should be just a build-time change. https://learn.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#controlling-dependency-assets

It should be possible to drop the IncludeAssets element too: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/DotNetCli.md

It's totally up to you whether you want to use StyleCop in v2, v3, or not at all. I think just the important thing is to get the style changes that it suggests in (whether that's enforced or not). Otherwise it will probably just be too much work to get bug fixes in that might turn up while working on v3.

Personally, I'm happy with things being dotnet 8 only - I have no requirement for anything else. I can't speak for others, but I'd be happy also supporting .NET Standard 2.0/2.1 if you wanted to do so. There would be a bunch of code that could be dropped with dotnet 8 only, and it would be easier to provide async methods. From a migration perspective, the only problematic thing I notice is that I haven't found a comparable Hex Editor control, there's only a viewer... https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.design.byteviewer?view=windowsdesktop-8.0

Did you want me to just drop the StyleCop config from the end of the PR for now if that's the only thing blocking the rest of it?

ironfede commented 1 month ago

@ironfede That's curious about the warning - I don't get it on mine. Maybe try a close, git clean -dfx and reopen, and see if it happens. I suspect that it's an environmental issue.

WRT. to introducing additional dependencies. the PackageReference has <PrivateAssets>all</PrivateAssets> to prevent StyleCop flowing through as a dependency to other projects. i.e. it should be just a build-time change. https://learn.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#controlling-dependency-assets

It should be possible to drop the IncludeAssets element too: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/DotNetCli.md

It's totally up to you whether you want to use StyleCop in v2, v3, or not at all. I think just the important thing is to get the style changes that it suggests in (whether that's enforced or not). Otherwise it will probably just be too much work to get bug fixes in that might turn up while working on v3.

Ok with style changes but StyleCop only in v3.

Personally, I'm happy with things being dotnet 8 only - I have no requirement for anything else. I can't speak for others, but I'd be happy also supporting .NET Standard 2.0/2.1 if you wanted to do so. There would be a bunch of code that could be dropped with dotnet 8 only, and it would be easier to provide async methods. From a migration perspective, the only problematic thing I notice is that I haven't found a comparable Hex Editor control, there's only a viewer... https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.design.byteviewer?view=windowsdesktop-8.0

Sorry, I've been too tranchant in the comment... Yes, I would keep support for netstandard 2.0 in order to allow usage. So net8 + netstandard20 in v3 Hexeditor is an issue... I start to investigate for alternative solutions.

Did you want me to just drop the StyleCop config from the end of the PR for now if that's the only thing blocking the rest of it?

Yes please

Thank you @jeremy-visionaid

Numpsy commented 1 month ago

Hexeditor is an issue... I start to investigate for alternative solutions.

If there is still a .NET Standard 2.0 build of the lib (rather than just making it .NET 8) then StructuredStorageExplorer could stay on ,NET 4.8, at least to begin with?

jeremy-visionaid commented 1 month ago

Yeah, I think if netstandard2.0 is being kept then the core libraries could just be made netstandard2.0 only.

IIRC, net48 is the only .NET Framework version that's currently supported (and is also covered by netstandard2.0), so net40 should just be dropped. StructuredStorageExplorer could then stay net48 and all the tests etc. could be net8.0.

I'd then perceive adding net8.0 to the core libraries as optional extra work - both paths would need to be supported but we'd be unable to take advantage of a lot of its new APIs etc. in the common code. I'm happy either way, but netstandard2.0 only might just be easier.

jeremy-visionaid commented 1 month ago

@ironfede I've dropped StyleCop config from the PR as requested

Numpsy commented 1 month ago

I'd then perceive adding net8.0 to the core libraries as optional extra work

Personally, I'd like an official build for .NET x.0 that can remove the dependency on the System.Text.Encoding.CodePages NuGet package - e.g. https://github.com/ironfede/openmcdf/pull/123 - which isn't itself much extra work (and then you also get better support for trimming/AOT analysis). Also, if we added any span based functionality then .NET Standard 2.0 will require System.Memory and related packages, and a .NET 8 target wouldn't