Merge SixLabors.Core sources into ImageSharp [first attempt] #1086

antonfirsov · 2020-01-19T04:26:44Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

Merge sources and tests from SixLabors.Core, resolve #1002.

Namespace structure were left untouched.
The new source structure follows namespace hierarchy in src.
SixLabors.Core tests have been grouped under their own directory
Most memory-intensive ArrayPoolMemoryAllocatorTests have been disabled temporarily. The safest way to run such tests would be arcane's RemoteExecutor

Do not squash-merge!

This reverts commit bdd54d3.

doing this to be consistent with System.Drawing.Primitives

… to make sure they can be used.

Added guards from the ImageSharp project

codecov · 2020-01-19T04:38:36Z

Codecov Report

Merging #1086 into master will increase coverage by 0.05%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
+ Coverage   83.71%   83.77%   +0.05%     
==========================================
  Files         688      705      +17     
  Lines       28853    29312     +459     
==========================================
+ Hits        24155    24555     +400     
- Misses       4698     4757      +59

Flag	Coverage Δ
#unittests	`83.77% <66.66%> (+0.05%)`	⬆️

Impacted Files	Coverage Δ
src/ImageSharp/ImageSharp/Formats/Tga/TgaFormat.cs	`83.33% <ø> (ø)`
...eSharp/Formats/Jpeg/Components/Block8x8F.CopyTo.cs	`100% <ø> (ø)`
...olorSpaces/Conversion/ColorSpaceConverter.Adapt.cs	`82.85% <ø> (ø)`
...rc/ImageSharp/ImageSharp/Formats/Bmp/BmpDecoder.cs	`100% <ø> (ø)`
...harp/Formats/Jpeg/Components/Decoder/JFifMarker.cs	`92.1% <ø> (ø)`
...plementation/Converters/CIeLchToCieLabConverter.cs	`100% <ø> (ø)`
...rp/ImageSharp/Formats/Png/Filters/AverageFilter.cs	`100% <ø> (ø)`
...lorSpaces/Conversion/ColorSpaceConverter.CieLuv.cs	`100% <ø> (ø)`
...arp/Formats/Jpeg/Components/Encoder/HuffmanSpec.cs	`100% <ø> (ø)`
...harp/Formats/Jpeg/Components/Encoder/HuffmanLut.cs	`100% <ø> (ø)`
... and 1156 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3ecd89...6ef017e. Read the comment docs.

JimBobSquarePants

I think we should go all in and merge the code under the collective SixLabors.ImageSharp namespace with folder structure.

It seems very odd to me not to have that as the default and developers will expect things like Point to be under the same root namespace as Image since System.Drawing groups objects like that together.

JimBobSquarePants · 2020-01-19T05:13:09Z

src/ImageSharp/GeometryUtilities.cs

+    /// <summary>
+    /// Utility class for common geometric functions.
+    /// </summary>
+    public static class GeometryUtilities


This is a naming convention I want to adopt throughout the codebase. Currently we've also got Utils and Helper(s)

JimBobSquarePants · 2020-01-19T05:14:15Z

src/ImageSharp/ImageSharp.csproj

    <PackageId>SixLabors.ImageSharp</PackageId>
    <PackageTags>Image Resize Crop Gif Jpg Jpeg Bitmap Png Core</PackageTags>
-    <RootNamespace>SixLabors.ImageSharp</RootNamespace>
+    <RootNamespace>SixLabors</RootNamespace>


You'll have to sell this change to me. Was it to reduce breaking changes?

JimBobSquarePants · 2020-01-19T05:17:31Z

src/ImageSharp/ImageSharp.csproj.DotSettings

 	<s:Boolean x:Key="/Default/CodeInspection/NamespaceProvider/NamespaceFoldersToSkip/=common/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/CodeInspection/NamespaceProvider/NamespaceFoldersToSkip/=common_005Cexceptions/@EntryIndexedValue">True</s:Boolean>
+	<s:Boolean x:Key="/Default/CodeInspection/NamespaceProvider/NamespaceFoldersToSkip/=imagesharp_005Cpixelformats_005Cpixelimplementations/@EntryIndexedValue">True</s:Boolean>
+	<s:Boolean x:Key="/Default/CodeInspection/NamespaceProvider/NamespaceFoldersToSkip/=imagesharp_005Cpixelformats_005Cutils/@EntryIndexedValue">True</s:Boolean>


I really don't want to keep dotsettings files, Resharper is a nightmare or performance. We should use .editorconfig + ruleset.

The other "toy IDE" 😆, Rider is also using this, and it's somewhat faster.
Still unsure what will be my IDE of choice in long term, but for now I'm pretty much addicted to JetBrains features, so I need .DotSettings.

"NamespaceProvider" is one of the features I really like btw.

JimBobSquarePants · 2020-01-19T05:26:54Z

tests/ImageSharp.Tests/_SixLabors.Core/Helpers/DebugGuardTests.cs

+        }
+
+        [Fact]
+        public void AllStaticMethodsOnOnDebugGuardHaveDEBUGConditional()


We don't need these tests. The shared code is already tested, though I should add a simple workflow to that repo.

https://github.com/SixLabors/SharedInfrastructure/blob/7730e662d25d78566aed7a6e2b78a16592feee7c/tests/SharedInfrastructure.Tests/DebugGuardTests.cs

Previously we used to test them in Core. If you setup an action in SharedInfrastructure, I'm happy to delete them here.

JimBobSquarePants · 2020-01-19T06:44:41Z

tests/ImageSharp.Tests/_SixLabors.Core/Memory/ArrayPoolMemoryAllocatorTests.cs

+namespace SixLabors.Memory.Tests
+{
+    // TODO: Re-enable memory-intensive tests with arcade RemoteExecutor:
+    // https://github.com/dotnet/runtime/blob/master/docs/project/writing-tests.md#remoteexecutor


antonfirsov · 2020-01-19T12:26:18Z

@JimBobSquarePants if we move the types, we should also do SixLabors.Shapes -> SixLabors.Drawing.Shapes in the drawing project for consistency. (cc @tocsoft)

I don't have a strong opinion apart from the fact that our decision should be user and business-focused now. So the question is: Does this worth the breaking changes (again) in beta7 -> RC1? Wouldn't it discourage users from adapting RC1?

JimBobSquarePants · 2020-01-19T13:51:03Z

I agree it should be user focused. That’s why I’m opting for creating the long term expected result over immediate upgrade.

I don’t think it’s too painful an upgrade anyway. The reference will remain and the user will be able to bulk delete the old namespace.

antonfirsov · 2020-01-19T13:56:34Z

That’s why I’m opting for creating the long term expected result over immediate upgrade.

OK 👍. Just to double check: this means that you also agree with the changes in Drawing, right? That one will have a bigger impact (on drawing users).

tocsoft · 2020-01-19T16:07:55Z

@anton changing the namespaces seems fine as an idea to me but I agree with @JimBobSquarePants and we should just totally drop the Primitives namespace part entirely and roll the types directly into the SixLabors.ImageSharp namespace. Memory on the other hand is more specialised and as people don't/wont be touching them regularly then SixLabors.ImageSharp.Memory makes sense for them.

re: drawing & shapes not sure exactly how to cut that namespace but as that project is not a target for our RC we can worry about cleaning us the structure over there at a later date.

antonfirsov · 2020-01-20T00:24:02Z

Closing in favor of #1087.

tocsoft and others added 30 commits June 2, 2017 21:55

Initial commit

a0eec28

initial primitive

035ddd3

update api key

c41e9e5

update sln

c309a2c

add matrix tests & fix GetHashCode

c14e346

Remove matrix in favour of Matrix3x2

01ac19b

Fix naming

0d865c9

Drop unneeded types plus added Size operators.

616a0d5

Merge branch 'master' of https://github.com/SixLabors/Primitives

3bd6c2a

remove opencover

03da628

rename project from Primitives to Core

55d2f65

add center property to rectangle

bdd54d3

add multiplication and divide operators to Point

0c5e364

Revert "add center property to rectangle"

04e70a1

This reverts commit bdd54d3.

drop zero in favor of empty

fa0b656

doing this to be consistent with System.Drawing.Primitives

remove IsEmpty based ToString() branches

81ffc07

Moved files in the Shared folder.

d47d55d

Whitespace

c3aa83b

Fixed version numbers

9e6ef4a

Renamed ruleset file.

ea36247

Enabled stylecop.

6271fd4

Fixed all stylecop warnings.

13dd85d

Changed copyright header.

6c78d2a

Moved HashHelpers to a folder.

d23742a

Fixed value in attribute.

46c3cba

Added guards from the ImageSharp project and added InternalsVisibleTo…

cc80def

… to make sure they can be used.

Switch to ReadOnlySpan.

af33ea4

Added SixLabors.ImageSharp.Drawing to the InternalsVisibleTo.

9d79164

Merge pull request #1 from SixLabors/feature/add-guards

ec9c338

Added guards from the ImageSharp project

push develop branch to an unstable feed for now

59cf965

antonfirsov added 12 commits January 19, 2020 00:02

Merge branch 'core' into af/merge-core

68d296c

move sources to subfolder

bcf41a7

fix T4 path in .csproj

0be3d5a

simlify T4 ItemGroups in .csproj

2f2a28f

merge Core sources

6314ae9

update SRCS package version

43b4531

move Core files

a68a963

move Core tests

4af6dab

fix build for tests

c8c3cee

delete old csproj

af221aa

fix numerical robustness issue in PointF test

33eb0e6

temporarily disable memory-intensive allocator tests

3833189

antonfirsov requested review from a team and JimBobSquarePants and removed request for JimBobSquarePants January 19, 2020 04:42

JimBobSquarePants added 3 commits January 19, 2020 16:10

Fix build version and disable test exitcode hack

24a164b

Filter VSTest targets

10b7872

Better codecov filtering

4b95654

JimBobSquarePants requested changes Jan 19, 2020

View reviewed changes

Update ImageSharp.Tests.csproj

6ef017e

antonfirsov changed the title ~~Merge SixLabors.Core sources into ImageSharp~~ Merge SixLabors.Core sources into ImageSharp [first attempt] Jan 20, 2020

antonfirsov mentioned this pull request Jan 20, 2020

Merge SixLabors.Core sources into ImageSharp #1087

Merged

4 tasks

antonfirsov closed this Jan 20, 2020

JimBobSquarePants deleted the af/merge-core branch April 24, 2020 11:06

Uh oh!

Merge SixLabors.Core sources into ImageSharp [first attempt] #1086

Merge SixLabors.Core sources into ImageSharp [first attempt] #1086

Uh oh!

Conversation

antonfirsov commented Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Uh oh!

codecov bot commented Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

antonfirsov Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

antonfirsov Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Jan 19, 2020

Uh oh!

JimBobSquarePants commented Jan 19, 2020

Uh oh!

antonfirsov commented Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tocsoft commented Jan 19, 2020

Uh oh!

antonfirsov commented Jan 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

antonfirsov commented Jan 19, 2020 •

edited

Loading

codecov bot commented Jan 19, 2020 •

edited

Loading

antonfirsov commented Jan 19, 2020 •

edited

Loading