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

feat: allow user implemented mappings to be marked as default #1071

Merged
merged 1 commit into from Feb 23, 2024

Conversation

latonz
Copy link
Contributor

@latonz latonz commented Jan 19, 2024

Fixes #1008

@latonz latonz self-assigned this Jan 19, 2024
@latonz latonz force-pushed the 891-primary-user-mapping branch 3 times, most recently from 81c0dda to 6615553 Compare January 25, 2024 06:57
@latonz latonz force-pushed the 891-primary-user-mapping branch 2 times, most recently from 2e2df29 to 9e95e38 Compare February 23, 2024 06:35
@riok riok deleted a comment from codecov bot Feb 23, 2024
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 94.31818% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 91.32%. Comparing base (4e4937c) to head (2080a7f).

Files Patch % Lines
src/Riok.Mapperly/Descriptors/MappingCollection.cs 92.13% 3 Missing and 4 partials ⚠️
...Riok.Mapperly.Abstractions/UserMappingAttribute.cs 0.00% 1 Missing ⚠️
...Mapperly/Configuration/UserMappingConfiguration.cs 66.66% 1 Missing ⚠️
.../Riok.Mapperly/Diagnostics/DiagnosticCollection.cs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1071      +/-   ##
==========================================
- Coverage   91.35%   91.32%   -0.04%     
==========================================
  Files         222      224       +2     
  Lines        7312     7447     +135     
  Branches      940      952      +12     
==========================================
+ Hits         6680     6801     +121     
- Misses        410      420      +10     
- Partials      222      226       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@latonz
Copy link
Contributor Author

latonz commented Feb 23, 2024

I'm not sure whether or not we should consider the first user implemented mapping as default if AutoUserMappings = false, wdyt @CommonGuy?

@latonz latonz marked this pull request as ready for review February 23, 2024 06:53
@latonz latonz enabled auto-merge (squash) February 23, 2024 07:09
@CommonGuy
Copy link
Contributor

Do we really need this? Let's take this as an example:

[UserMapping]
public static int Method1(int s) => s;

[UserMapping(Default = true)]
public static int Method2(int s) => s;

// some partial mapping method would follow here

Is there any scenario where Method1 will ever be used as a user mapping by Mapperly? As far as I can see, Method2 will always be picked. We could use [UserMapping(Ignore = true)] on Method1 here, which would have the same effect.

But maybe there are some scenarios (generic mapping method?) where multiple user methods could apply? If that is the case, then this new feature makes sense, but those scenarios should be used in the tests instead of mapping from int to int.

Instead of using a Default property, maybe an Order or Priority property (default 0, higher priority is picked first) would make more sense? Let's imagine three user implemented methods:

  • Method1 with Priority = 1
  • Method2 with Priority = 2
  • Method3 with Priority = 3

For a partial method "MyMethod1", all three methods are suitable, so Method 3 is picked. For another partial method "MyMethod2", only Method1 and Method2 are suitable, so Method2 is picked. This wouldn't work if only Default = true could be defined on Method3.


Regardless of implementation, IMO the behavior should be:

  • One matching method -> everything is fine
  • Multiple matching methods, one has a default/best priority -> everything is fine
  • Multiple matching methods with default=false/priority=0 -> emit a warning, even though this is a slightly breaking change if someone has "Report Warnings as Errors" enabled. Relying on method ordering is dangerous and this is fixed easily
  • Multiple matching methods with multiple Default = true/same priority >0 -> emit an error

I don't think there is an advantage to making Default a nullable boolean, could default it to false.

@latonz
Copy link
Contributor Author

latonz commented Feb 23, 2024

Using default for now instead of priority as we do it in other parts (eg. FormatProvider). For now there is no real reason to use priority. If we want to introduce a priority concept we should do this consistently across the entire Mapperly API surface (for example including FormatProvider and ObjectFactory). This could later be introduced without a breaking change by assigning a default priority value to instances which have Default = true set.

@latonz latonz merged commit 79f60cd into riok:main Feb 23, 2024
19 checks passed
@latonz latonz deleted the 891-primary-user-mapping branch February 23, 2024 08:07
Copy link

github-actions bot commented Mar 9, 2024

🎉 This PR is included in version 3.5.0-next.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PrimaryMappingAttribute to avoid accidentally using the wrong method for mapping an entity.
2 participants