wo80 / AcoustID.NET

AcoustID fingerprinter and webservice access in .NET
39 stars 17 forks source link

Migrate library to .NET Standard #5

Closed Jimmyson closed 4 years ago

Jimmyson commented 4 years ago

Looking to utilize the library in a .NET Core application, and thought it would be useful to have this migrated, supported and published to NuGet.

Namespaces have been preserved, and dependent libraries have been updated. Unit tests still pass. Removed Example WinForms application as no code remained.

wo80 commented 4 years ago

Thanks, I fixed the appveyor test runner.

Two remarks:

You made a lot of previously internal classes public. Is this because of the tests or is there any other reason? If it's because of the tests, you could add

[assembly: InternalsVisibleTo("AcoustID.Tests")] // or AcoustID.Core.Tests

to one of the source files and revert the changes (public -> internal).

Additionally, I don't like the Core suffix too much. Any good reason to add it to the project file names?

Jimmyson commented 4 years ago

My reason for adding Core was to indicate the change to Core framework, it was only in name though and I can revert this.

In regards to opening up the internal classes, I had access issues. I did this some time ago, and did not know about the assembly directive.

I can reattempt this to keep in line with access structure...

wo80 commented 4 years ago

If you need access to those classes for your work, I don't mind making them public. I was just wondering ...

But I'd prefer if you remove Core from directory and project file names. Thanks!

Jimmyson commented 4 years ago

I have taken a branch of the original fork, and followed through with checking the to see if it's possible to expose the Internal classes. And the tests successfully pass.

image

There is an attribute that can be added to the primary library's CSPROJ file that allows the "InternalVisableTo" flag

<ItemGroup>
    <AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo">
        <_Parameter1>$(AssemblyName).Tests</_Parameter1>
    </AssemblyAttribute>
</ItemGroup>

https://blog.sanderaernouts.com/make-internals-visible-with-new-csproj-format

I can revert the file back to their original state, but since I changed the folder name to include the Core value. I will need to rollback the branch. Apologies for that.

Jimmyson commented 4 years ago

I have rolled back my changes and reattempted the migration. Preserving the Internals and removing the Core name from folders too. I wasn't too sure about if you were also concerned about the test and src folders, so reverted that too.

But I have now implemented the defined changes you have requested for Internals and the property group

wo80 commented 4 years ago

Ok, this looks much cleaner. I'll merge and publish a new package this weekend.