Skip to content
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

Retarget aghacks with dotnet standard 1.3 as a minimum #107

Merged
merged 1 commit into from
Aug 7, 2020

Conversation

Marv51
Copy link
Contributor

@Marv51 Marv51 commented Aug 6, 2020

I cleaned up the compatibility classes and attributes in AgHacks.cs, assuming that .net standard 1.3 is currently the lowest supported version. This removes any compatibility hacks from the .net standard 2.0 build.

This fixes #102, #106 and #76 (or at least it should)

It builds and all the tests pass, I'm not familiar with what other kind of testing is required to make sure all supported versions work well with this change.

Maybe @max-brightspark knows something about this? He changed the IConvertible implementation in 2018.

@dejx
Copy link

dejx commented Aug 7, 2020

Looks good, another approach could be by using this interfaces only on silverlight platform, but this means targeting netstd 1.3 in all other projects.
E.g. https://github.com/dejx/PdfSharpCore/pull/1/files

@Marv51
Copy link
Contributor Author

Marv51 commented Aug 7, 2020

I believe the minimum requirement for this repo is .net standard 1.3 (at least it was broken on anything below that).

So I think #if SILVERLIGHT will be always false. BrowsableAttribute on the other hand is not available in standard 1.3 so the compatibility implementation is still needed.

@dejx
Copy link

dejx commented Aug 7, 2020

Cool. All good with me. @ststeiger when can we expect new package? :)

@ststeiger ststeiger merged commit f8002dd into ststeiger:master Aug 7, 2020
@ststeiger
Copy link
Owner

ststeiger commented Aug 7, 2020

@Marv51: Merged.
@dejx: The package should be automatically rebuilt as we speak.

Why did you remove
uint System.IConvertible.ToUInt32(IFormatProvider provider)
etc.

That was a fix for a bug that I ran into, too.
Is this no longer required with a higher netstandard-version, conflicting ?
Or did you just delete it ?

@ststeiger
Copy link
Owner

ststeiger commented Aug 7, 2020

Here's the corresponding issue for the change in IConvertible:
#2

and the corresponding pull-request:
#8

@Marv51
Copy link
Contributor Author

Marv51 commented Aug 7, 2020

The compatibility hack (I believe) works like this:
If the framework provides System.IConvertible, then the interface declaration references that.
If System.IConvertible is not available (like on silverlight) AgHacks provides a PdfSharp.IConvertible interface.

Because the #if's in the AgHacks where not working correctly for pdfsharpcore, builds with both PDFSharp.IConvertible and System.IConvertible were built. Leading the compiler to choose the PDFSharp version of the interface eventhought the System one is available.

IConvertible is available very broadly today (https://docs.microsoft.com/en-us/dotnet/api/system.iconvertible), so I removed the compatibility version of the interface (Leading the other code to always implement System.IConvertible)

@ststeiger
Copy link
Owner

OK, we'll see if it works, if it does, all fine with me.

@Marv51 Marv51 deleted the modernize-aghacks branch August 10, 2020 09:15
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.

SerializableAttribute, BrowsableAttribute and ICloneable overwritten in netstandard 2.0
3 participants