8000 Solution/Project update by jeremy-visionaid · Pull Request #156 · ironfede/openmcdf · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Solution/Project update #156

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

Merged
merged 7 commits into from
Sep 18, 2024

Conversation

jeremy-visionaid
Copy link
Collaborator

This does a few things:

  • Adds a default editorconfig (and updates to the IDE editor's defaults - who knows why they're different!)
  • Adds a default spell checking config and exclusions
  • Adds solution-wide common build properties and targets
  • Updates the language version to C# 10 (since some projects are now dotnet 6, so it gives consistency across target frameworks)
  • Enables enforcement of code style at build time
  • Adds StyleCop for extended style analysis/enforcement

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
Copy link
Owner

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
Copy link
Collaborator Author
jeremy-visionaid commented Sep 16, 2024

@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
Copy link
Owner

@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
Copy link
Contributor
Numpsy commented Sep 17, 2024

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
Copy link
Collaborator Author
jeremy-visionaid commented Sep 17, 2024

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
Copy link
Collaborator Author

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

@Numpsy
Copy link
Contributor
Numpsy commented Sep 18, 2024

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

@ironfede ironfede merged commit 56eff33 into ironfede:master Sep 18, 2024
@jeremy-visionaid jeremy-visionaid deleted the project-update branch October 16, 2024 07:36
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.

3 participants
0