-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Cleanup ifdefs and revise hashing algorithm #503
Conversation
lol - looks like I forgot to submit my self review last night! |
I'm heading out now but if you read my comments (which I forgot to post last night), I think it will answer some of your questions. I should be able to answer any other questions by tomorrow. |
Sure, will check your comments. Thanks again! :-) |
One more thing (besides #503 (review)) before merging this, is the compatability matrix: https://github.com/codebude/QRCoder/wiki/Advanced-usage---QR-Code-renderers#2-overview-of-the-different-renderers Has anything changed due to the updated ifdefs? (I would say no after reviewing the PR, but maybe I've overseen something.) |
Having a look onto the compatability matrix, I asked myself why e.g. PostscriptQRCode isn't compatible with .NET Standard 1.3. I checked the history/blames and saw that the exclusion ifdef intially was defined as I wish I had commented back then which feature/function was missing in PCL when I excluded it, so that I could check now if its available in .NET Standard 1.3. I guess I have to simply try out... |
Got it. It's System.Drawing not beeing availble for NET_STANDARD1_3. So ignore my last comment. |
No; this doesn't change the exposed API. If you like, I can help write a test that produces a copy of the API so it is easy to tell when it changes. So it will auto generate a file like this: In our case at GraphQL.NET, we tailored the routine to be specific per-tfm, which complicates the routine considerably but would be particularly useful here where you have different APIs for various tfms. We also extended the routine to auto update when it fails, so it is simpler for devs. |
Correction: with the exception of adding the methods that were accidentally missing from .NET 6 |
|
That looks great! I've seen that you already started in #504 - thanks! :-) |
Certainly! #504 won't change the compiled code. |
Cleans up ifdefs
To the extent possible:
SYSTEM_DRAWING
is used for when System.Drawing.Common is availableNET6_0_OR_GREATER
is used to applySupportedOSPlatformAttribute
(maybe in future PR this is polyfilled)!NETSTANDARD1_3
is used to exclude code that requires System.Drawing.PrimitivesAnd for tests:
SYSTEM_DRAWING
is used for when System.Drawing.Common is availableTEST_XAML
is used for XAML tests!NETCOREAPP1_1
is used to exclude tests for .NET Standard 1.1, and for hash checks for PNG testsAll other ifdefs are simplified and used as minimally as possible (e.g.
NETFRAMEWORK
vsNET35 || NET40
)Also, many tests were not performed on .NET 6, probably since the hashes don't match due to a new deflate algorithm. Worse yet, they do not appear to be deterministic, so rerunning the test can produce a different hash. I've changed the hashing algorithm to be deterministic based on the pixel values inside the
Bitmap
. This only works on platforms with access toBitmap
so the PNG tests still have ifdefs when running on .NET Core 1.1.