Multitarget main library and its tests

KirillOsenkov / XmlParser

A Roslyn-inspired full-fidelity XML parser with no dependencies and a simple Visual Studio XML language service

Apache License 2.0

328 stars 49 forks source link

Multitarget main library and its tests #52

Closed DoctorKrolic closed 11 months ago

DoctorKrolic commented 11 months ago

Made MS.Language.Xml multitarget .NET Standard 2.0 and all currently supported .NETs
Fixed several warning reported for higher versions of .NET
Updated System.Collection.Immutable reference to the latest version and scoped it to netstandard2.0 since it is included to the standard library for modern .NETs by default
Multitargeted test project for MS.Language.Xml and made it run .NET tests (this includes adding Microsoft.NET.Test.Sdk package reference since .NET test cannot be run without it)
Updated xunit versions to the latest available

KirillOsenkov commented 11 months ago

Could you explain why is this needed?

DoctorKrolic commented 11 months ago

When I did this I was certain that even on modern .NETs there will be System.Collections.Immutable.dll in the final output since version required by the library and the one included in the framework are too different (1.5.0 vs 8.0.0 on .NET 8, which is many major versions difference). Thus the main goal of the PR was to conditionally exclude System.Collections.Immutable from the dependency list and unlock other potential updates like this in the future (e.g. replace self-made hashcode combiner with now built-in HashCode struct by conditionally referencing Microsoft.Bcl.HashCode on .NET Standard and using a built-in solution on .NET 6+). You question made me question my certainty and actually check it. As it turned out, my initial assumption is actually not correct making this PR irrelevant. So I'll close it. Will extract changes related to warnings into a separate PR

KirillOsenkov commented 11 months ago

I feel like those are all valid points, but we could solve them one by one. For example for Hashcodes I like value tuples, e.g. (A, B, C).GetHashCode(). We could bump Immutable to latest, or maybe copy paste a couple types and delete the dependency altogether. I think our usage of Immutable is very modest.

KirillOsenkov commented 11 months ago

Yes, the only thing we use Immutable for is ImmutableArray:

I'd be happy to either switch to a regular .NET array or collection (like IReadOnlyList) or copy-paste the ImmutableArray source (doesn't seem worth it) or bump the old 1.2.3.0 version to anything more modern such as 6.0.0.

KirillOsenkov commented 11 months ago

I don't see a lot of value in using ImmutableArray