Skip to content

Add partial method modifier support for Value, From and TryFrom#847

Merged
SteveDunn merged 4 commits intoSteveDunn:mainfrom
DPDmancul:feature/partial-modifier-support
Oct 5, 2025
Merged

Add partial method modifier support for Value, From and TryFrom#847
SteveDunn merged 4 commits intoSteveDunn:mainfrom
DPDmancul:feature/partial-modifier-support

Conversation

@DPDmancul
Copy link
Contributor

This solves #845 for Value, From and TryFrom. It could be easily extended to support also TryParse and other methods.

This enables:

  • Adding an attribute to a generated method
[ValueObject]
public sealed partial class MyVo
{
    [Obsolete("For internal use only")]
    public static partial MyVo From(int value);
}
  • Making a generated method private
[ValueObject]
public sealed partial class MyVo
{
    private static partial MyVo From(int value);
}

@SteveDunn
Copy link
Owner

Thanks for the PR @DPDmancul . This is for C# 14 right? I guess we'll need to determine the version of Roslyn loaded and or the language version, before generating the partial methods.

@DPDmancul
Copy link
Contributor Author

DPDmancul commented Oct 3, 2025

This is for C# 14 right?

I ran the tests with C# 12, but it should work also for lower versions. Only for using the partial keyword with properties (e.g. Value) it is required C# 13. But this shouldn't be a problem: if the user is using a language version which doesn't allow the partial keyword, it will be notified with an error in its code.

we'll need to determine the version of Roslyn loaded [..] before generating the partial methods.

Indeed the problem in the CI tests is that the IPropertySymbol.IsPartialDefinition property is not found: this property is available since Roslyn 4.12.2

Since the version of Microsoft.CodeAnalysis.CSharp is not specified, it is taken from the target framework. So maybe this could be solved with preprocessor directives, putting the logic inside an #if NETX_0_OR_GREATER, and otherwise returning always null (as if the partial property was not found).

@DPDmancul DPDmancul force-pushed the feature/partial-modifier-support branch from 2cf0d99 to dce1621 Compare October 4, 2025 09:15
@DPDmancul DPDmancul changed the title Add partail method modifier support for Value, From and TryFrom Add partial method modifier support for Value, From and TryFrom Oct 4, 2025
@DPDmancul DPDmancul force-pushed the feature/partial-modifier-support branch from dce1621 to 393b4a5 Compare October 4, 2025 09:30
@DPDmancul
Copy link
Contributor Author

Since there is no direct correspondence between target framework and Roslyn versions, I added a runtime fallback. Instead of directly calling the property, I try to get the property with reflection and return a fallback value (false) if it does not exist (i.e. we have an old Roslyn).

@DPDmancul DPDmancul force-pushed the feature/partial-modifier-support branch from 1055712 to f99add9 Compare October 4, 2025 13:32
@DPDmancul DPDmancul force-pushed the feature/partial-modifier-support branch from f99add9 to d2659ee Compare October 4, 2025 22:31
@SteveDunn
Copy link
Owner

I think we're nearly there @DPDmancul ! Thanks for your hard work on this one; apologies, I know it's quite difficult even to get the simplest of changes into Vogen!

It looks like the last hurdle is to fix the snapshot tests. You can run them from the IDE, or from the command line. If I'm fairly certain that my changes are all correct, I run .\RunSnapshots.ps1 -v "minimal" -reset in the root. This takes about 30 minutes and rebases the snapshots to what is generated on that run. It's a good gauge of the impact of your change: if you see only a small number of git changes, then your fix is limited and hasn't modified anything else.

@SteveDunn SteveDunn requested review from SteveDunn and removed request for viceroypenguin October 5, 2025 12:54
Copy link
Owner

@SteveDunn SteveDunn left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the great contribution!

@SteveDunn SteveDunn merged commit 8cd7fd6 into SteveDunn:main Oct 5, 2025
6 checks passed
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.

3 participants