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

Added support for some immutable collections #1345

Open
wants to merge 1 commit into
base: release/5.0.0
Choose a base branch
from

Conversation

Romfos2
Copy link

@Romfos2 Romfos2 commented May 8, 2022

Description

Added support for ImmutableArray, ImmutableList, ImmutableDicrionary

Closed issues

n\a

Checklist

  • Reviewed the contribution guidelines
  • Linked the issue(s) the PR closes
  • Implemented automated tests and checked coverage
  • Provided inline documentation comments for new public API
  • Ran the full solution build and validation locally

@Romfos2
Copy link
Author

Romfos2 commented May 8, 2022

Hi,

@aivascu could you please review it and publish pre-release nuget package for v5 on nuget.com?

Thank you

@Romfos2
Copy link
Author

Romfos2 commented May 11, 2022

cc @Kralizek

@aivascu
Copy link
Member

aivascu commented May 17, 2022

@Romfos2 thank you for the PR.

I'm honestly not sure if I should add the support for immutable collections, into the base package of AutoFixture.
While I can motivate the support for IReadOnlyCollection<> by saying that it's in the BCL, I cannot do the same for types that live in a separate package. Are these types commonly used in some framework?

As I can see there is a community package AutoFixture.Community.ImmutableCollections, which adds support for the same collections, but is a bit outdated. Have you tried to reach the author of that package in order to update it?

If he doesn't answer I'll consider creating a new AutoFixture package that adds support for immutable collections.

@Kralizek I'm interested to know what you think.

@Kralizek
Copy link
Contributor

@aivascu i agree with you that the AF base package should have no extra dependency and offer support to only those contained in the basic packages.

It is worth discussing whether or not other Microsoft types should be left to the community to support or should be supported by this organization.

Ploeh some time ago defined AF feature complete and, besides a general facelift of the API/developer experience and support for new constructs introduced in the C# language and .NET runtime, I tend to agree.

Accepting AF being basically feature complete means we now have bandwidth to improve support for glue libraries (e.g. the NUnit one) and for specimen builders of new types being introduced and becoming more and more common.

On the other hand, I would avoid bloating even more the already gargantuan AF repo/solution.

Maybe we could think of a Extras repository where packages AutoFixture.Extras.* can be placed and evolve at a different pace from the main repo.

@aivascu
Copy link
Member

aivascu commented May 17, 2022

@Kralizek I agree, there should be a separate kind of packages that add support for common features, that cannot be supported by the official AutoFixture libraries. The "Extras" prefix, is a good candidate to group those packages. It sounds intuitive and is not that long to spell. 🙂

Community support is tricky since people will often create the packages at a point in time when they need those packages and then abandon them, once they move on to different projects/technology stacks. Same happens with PRs and Issues. So while it is nice to have more community packages, they are not as reliable, and the ImmutableCollections package might be a good example.

This actually makes me think that perhaps there should be a separate organization for community packages. This would allow to recycle community packages, that are not supported anymore by their original authors, but due to their name or popularity might be too big to die. Something like the .NET Foundation but for AutoFixture. 😄

@Kralizek
Copy link
Contributor

Why not an extras repo in the AF org?

@aivascu
Copy link
Member

aivascu commented May 17, 2022

@Kralizek yes for Extras there would be a separate repo in the AF organization. I was talking about a separate org in the context of the abandoned AutoFixture.Community.* packages.
However the authors would have to opt-in explicitly and transfer the packages to that org.

@Romfos
Copy link

Romfos commented May 21, 2022

@aivascu @Kralizek System.Collections.Immutable is part of BCL in .NET Core, but not in .NET Framework or .NET Standard

I think it should be out of the box because .Net Core is future (and most used runtime already)

System.Collections.Immutable

Product Versions
.NET Core 1.0, Core 1.1, Core 2.0, Core 2.1, Core 2.2, Core 3.0, Core 3.1, 5, 6, 7 Preview 3
UWP 10.0

for .NET Framework or .NET Standard support provided by nuget package

@Kralizek
Copy link
Contributor

Thank you for your clarification.

I think the difference between these classes and IReadOnlySet<T> is that the latter needed only a relay to be supported natively. Also that type is not available at all before .NET .5.

The same doesn't apply for the immutable collections.

Options would be:

  • native support for .net core, external package for .net framework
  • native support for both runtimes with external dependency in .net framework
  • external package for both runtimes

When it comes to my personal preference:

  • i believe that the .net core runtime is the future and should be supported at its best
  • whenever possible, i like symmetry between the support natively offered between the two runtimes (it would be weird that a class is supported natively in netcore and requires an extra package in netfx)

That being said, I'm not a maintainer, just a very vocal user with tiny contributions.

I guess @aivascu should pitch in here :)

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.

None yet

4 participants