-
Notifications
You must be signed in to change notification settings - Fork 7
Add user defined mapping #104
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
base: master
Are you sure you want to change the base?
Add user defined mapping #104
Conversation
| @@ -0,0 +1,38 @@ | |||
| namespace Zomp.SyncMethodGenerator; | |||
|
|
|||
| internal sealed class UserMappings( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should rename this class to UserMappingResult or something else.
Plural class names are kinda cursed 🙂
| var progress = new global::System.Progress<float>(); | ||
| WithIProgress(progress); | ||
| } | ||
| //HintName: Test.Class.CallWithIProgressAsync.g.cs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah.. I had the issue again with line endings. I've disabled autoclrf in Git and replaced all line endings, but it seems were also wrong.
Should I revert these changes? Or is it OK that they are in this PR?
| var result = driver.GetRunResult().Results.Single(); | ||
| var sourceOutputs = | ||
| result.TrackedOutputSteps.SelectMany(outputStep => outputStep.Value).SelectMany(output => output.Outputs); | ||
| var (value, reason) = Assert.Single(sourceOutputs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was failing because there are now 2 context.RegisterSourceOutput's (for the mapping diagnostics and method results). That's why I check for the output length 2 now.
| title: "Attribute and user mapping conflict", | ||
| messageFormat: "Method '{0}' has both an attribute and a user mapping defined. The user mapping will be used.", | ||
| category: Preprocessor, | ||
| DiagnosticSeverity.Error, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to have [CreateSyncMethod] and an user mapping on a method:
- The sync method will still be created. You can directly call it.
- However, whenever the async-to-sync rewriting happens, the user mapping will be used.
To prevent confusion why the method isn't being called, I've made this an error.
|
may be it would be benefitical to use custom xml nodes inside Directory.Build.props files instead of brand new SyncMethods.txt. the benefits:
just an idea =) |
| }); | ||
| } | ||
|
|
||
| private static UserMappings GetUserMappings(ImmutableArray<(string Path, string Content)> array, CancellationToken token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
may be it would be better to move this this inside an UserMappings class and make public and change it name to Create?
|
It's possible to add the following in the Directory.Build.props: <AdditionalFiles Include="$(MSBuildThisFileDirectory)SyncMethods.txt" />And then create a To add extra methods to a single project, you can create a SyncMethods.txt in the project folder and add the following: <AdditionalFiles Include="SyncMethods.txt" />Then those two files will be combined (Directory.Build.props + .csproj). Custom XML nodeIf we want to make custom node, we're limited to a single value (or we have to go through the files ourself, but IO access in a source generator is not recommended by Microsoft). In the .csproj/.props the following have to be added (otherwise we don't have access to this node): <ItemGroup>
<CompilerVisibleProperty Include="SyncMethods" />
</ItemGroup>Then we can read out the SyncMethods in the source generator. For example: <PropertyGroup>
<SyncMethods>
System.Threading.Tasks.Task.Delay=Test.CustomThread.Sleep
</SyncMethods>
</PropertyGroup>I'm not sure if we're allowed to have inner XML nodes in the property. I haven't test this. If this is possible, then we can use XML nodes. Let's say the example above was defined in the .props-file, and we want to extend the methods in the project (.csproj), then you'll have to add <PropertyGroup>
<SyncMethods>
$(SyncMethods)
System.Threading.Tasks.Task.Delay=Test.CustomThread.Sleep
</SyncMethods>
</PropertyGroup>I'm fine with both. I chose the .txt-file because this was discussed in #92 and CsWin32 (a project from Microsoft) uses NativeMethods.txt. |
|
I'm have no strong opinion for this, I just only highlighed idea. I guess you and @virzak should make decision about the implementation. =) Let me add 3 additional point:
As I said above, the decision is yours and @virzak. Anyway, this is a cool idea 👍 |
This is a good point. Another thing is, for example, EF Core has I'll add a test case for this.
Not really, I'm not the project owner on this one; I'm just a contributor 😉 |
| return obj is not null && (ReferenceEquals(this, obj) || (obj is UserMappings other && Equals(other))); | ||
| } | ||
|
|
||
| public override int GetHashCode() => HashCode.Combine(Mappings); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public override int GetHashCode() => HashCode.Combine(Mappings); | |
| public override int GetHashCode() => HashCode.Combine(mappings, Diagnostics); |
Typo; this is using the dictionary instead of the EquatableArray, and is missing the diagnostics.
Fixes #92
This PR adds
SyncMethods.txtsupport.In the project, the following has to be added to the .csproj:
After that, the mappings will be used. The syntax is as following:
Priority
User mappings have always priority. For example, if you do add the following mapping:
Normally
System.Threading.Thread.Sleepwould be used if you doTask.Delay(...), but since there is a user mappingTest.CustomThread.Sleepwill be used in the current project.Checks
The source generator validates the following:
[CreateSyncVersion]and an user mapping on the method -> ZSMGEN005