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

Add option to generate specialized partition implementation #15

Merged
merged 19 commits into from
Oct 1, 2024

Conversation

Mafii
Copy link
Member

@Mafii Mafii commented Sep 17, 2024

No description provided.

@Mafii Mafii requested a review from bash September 17, 2024 14:04
@Mafii Mafii added the enhancement New feature or request label Sep 17, 2024
@bash
Copy link
Member

bash commented Sep 17, 2024

This looks like a useful addition!

Extension class

I'm a bit unhappy with the separate extension class because it's a new, non-configurable name which lives top level, thus it's totally disconnected from the namespacing of potentially nested unions.

The first is easy to fix (doesn't have to be done now as it is easy to add later in a non-breaking manner). The second is a bit trickier. Given the following example:

public class A
{
	[DiscriminatedUnion(...)]
	public abstract record B 
	{
		// ...
	}
}

[DiscriminatedUnion(...)]
public abstract record B 
{
	// ...
}

If you enable Partition on both top-level and nested B unions, the extension class has to be generated top-level for both (Is that the case currently? If not this needs to be changed. Quoting the compiler error: "Extension methods must be defined in a top level static class").

However, both extension classes are named BEnumerableExtensions and thus conflict.
The easiest solution that I can think of is making the extensions class partial and relying on overloading.

Partitions record

I know we use dedicated structs in Funcky for the partitions. However I'm wondering if that's the right choice here.
Records come with quite a bit of generated members themselves which in this case are double-hidden (once by the record keyword and once since the record itself is generated). Maybe returning a named tuple is better here?


(I'm not very active w. Polyadic these days, so feel free to overrule me)

@Mafii Mafii force-pushed the partition-specialization branch 2 times, most recently from d691686 to 3e97880 Compare September 25, 2024 11:41
@Mafii Mafii requested a review from bash September 25, 2024 11:46
@Mafii Mafii requested a review from bash October 1, 2024 08:43
Copy link
Member

@bash bash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🎉

Do you have permissions to publish the NuGet package?

Copy link
Member

@bash bash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, you forgot to update the docs in the readme :)

@bash bash merged commit 8d8d375 into polyadic:main Oct 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants