-
-
Notifications
You must be signed in to change notification settings - Fork 152
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: add MapDerivedType
for existing target type mapping
#918
Conversation
d157087
to
175df38
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #918 +/- ##
==========================================
+ Coverage 91.29% 91.38% +0.08%
==========================================
Files 222 223 +1
Lines 7165 7239 +74
Branches 907 909 +2
==========================================
+ Hits 6541 6615 +74
Misses 412 412
Partials 212 212 ☔ View full report in Codecov by Sentry. |
I intentionally kept |
11dec6b
to
0424be5
Compare
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.
Thank you for this PR! 😊 I added my feedback 😊
src/Riok.Mapperly/Descriptors/MappingBuilders/DerivedTypeMappingBuilder.cs
Outdated
Show resolved
Hide resolved
src/Riok.Mapperly/Descriptors/Mappings/DerivedExistingTargetTypeMapping.cs
Outdated
Show resolved
Hide resolved
src/Riok.Mapperly/Descriptors/Mappings/DerivedExistingTargetTypeMapping.cs
Outdated
Show resolved
Hide resolved
src/Riok.Mapperly/Descriptors/Mappings/DerivedExistingTargetTypeMapping.cs
Outdated
Show resolved
Hide resolved
src/Riok.Mapperly/Descriptors/Mappings/DerivedExistingTargetTypeMapping.cs
Outdated
Show resolved
Hide resolved
src/Riok.Mapperly/Descriptors/Mappings/DerivedExistingTargetTypeMapping.cs
Outdated
Show resolved
Hide resolved
src/Riok.Mapperly/Descriptors/Mappings/TypeMappingBuildContext.cs
Outdated
Show resolved
Hide resolved
src/Riok.Mapperly/Descriptors/Mappings/DerivedExistingTargetTypeMapping.cs
Outdated
Show resolved
Hide resolved
9673fbb
to
8af73a1
Compare
Thanks for the review 👍 I think I've made your changes, really liked the simplification of Opted to not create |
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.
Thank you for the updates, looks almost ready to merge, few smaller things I noted.
Also an integration test should be added and the documentation should be updated (as you already noted in the PR description ToDo-List 😊 )
src/Riok.Mapperly/Descriptors/MappingBuilders/DerivedTypeMappingBuilder.cs
Outdated
Show resolved
Hide resolved
5b2274f
to
e8ee438
Compare
Can't (easily) do the integration tests rn because of a weird bug - can't find the correct file in the debug artifacts folder. Which docs section should I update? |
Can you elaborate on the bug? Probably run
I'd add a sentence in |
02418ec
to
2e5e8fa
Compare
b536683
to
b2d8031
Compare
b2d8031
to
0251794
Compare
Thanks
👍 |
0251794
to
d6bd92b
Compare
Looks like the benchmark check is failing. Can't run a benchmark right now, but I don't think this PR should have a massive impact on performance. |
@TimothyMakkison not sure what we should do about the benchmark test, it's failing quite often recently without any notable performance issues. |
d6bd92b
to
483ff13
Compare
Strange 🤷 perhaps github pipelines are running slow? For now we could change the threshold percentage |
@TimothyMakkison created #969 to increase the threshold. |
0aec783
to
da6c1ff
Compare
da6c1ff
to
a338626
Compare
Thanks for the fixing the benchamark, no clue why the integration failed. Hopefully that's the end of the CI issues 🤞 although code coverage is being weird. |
@TimothyMakkison codecov has been super unreliable lately, I think some other open source projects replaced it recently. I created #930 a few days ago to look for alternatives and probably switch to another coverage provider or remove it completely. If you know a good solution, please add a comment there 😊 |
🎉 This PR is included in version 3.3.0-next.6 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 3.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
feat: partially add
MapDerivedType
for existing target type mappingDescription
Partially add
MapDerivedType
for void mapping methods. Doesn't support object (runtimeTargetTypeMapping???) or generic mapping, I'll do it in a separate PR.Example generated code
Fixes #694
Checklist