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: introduce experimental full nameof support #518

Merged
merged 1 commit into from
Aug 8, 2023
Merged

feat: introduce experimental full nameof support #518

merged 1 commit into from
Aug 8, 2023

Conversation

latonz
Copy link
Contributor

@latonz latonz commented Jun 23, 2023

introduce experimental full nameof support

Description

Introduces experimental full nameof support.
See #401.

This would allow simpler MapProperty attributes for nested properties:
[MapProperty(nameof(@A.B.C), nameof(@D.E.F)] would be the same as [MapProperty(new[]{nameof(A.B), nameof(A.B.C)}, new[]{nameof(D.E), nameof(D.E.F)}].

Checklist

  • The existing code style is followed
  • The commit message follows our guidelines
  • Performed a self-review of my code
  • Hard-to-understand areas of my code are commented
  • The documentation is updated (as applicable)
  • Unit tests are added/updated
  • Integration tests are added/updated (as applicable, especially if feature/bug depends on roslyn or framework version in use)

@latonz latonz self-assigned this Jun 23, 2023
@TimothyMakkison
Copy link
Collaborator

Nice, what made you reconsider this?

I'm not sure if it's possible, but is it possible to check for and warn the user when a qualified namespace is used?

@latonz
Copy link
Contributor Author

latonz commented Jun 23, 2023

@TimothyMakkison this is just an experiment 😊 I think the current solution is really tedious DX, however I'm not yet convinced by this solution. I just wanted to poke around and see what it gets. It is still far from ready to be merged, there is still a lot to consider and work out. Not sure if it should relay on the @ prefix, how to handle the namespace thing, ..., ... Not sure yet if all of this can be worked out with the existing roslyn APIs.

@TimothyMakkison
Copy link
Collaborator

TimothyMakkison commented Jun 23, 2023

I thought the @ symbol is a pretty clever solution - it prevents accidental usage and indicates somethings different.

The namespace thing isn't a big deal. Hopefully the error and documentation would inform people.
Perhaps when a lookup fails mapperly could scan all available assemblies and types looking for a matching namespace. Not sure how slow this is

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #518 (fbb4d49) into main (630fd17) will decrease coverage by 0.12%.
Report is 2 commits behind head on main.
The diff coverage is 85.59%.

@@            Coverage Diff             @@
##             main     #518      +/-   ##
==========================================
- Coverage   90.90%   90.79%   -0.12%     
==========================================
  Files         170      171       +1     
  Lines        5905     5997      +92     
  Branches      754      764      +10     
==========================================
+ Hits         5368     5445      +77     
- Misses        371      377       +6     
- Partials      166      175       +9     
Files Changed Coverage Δ
...scriptors/InlineExpressionMappingBuilderContext.cs 95.91% <ø> (ø)
...Riok.Mapperly/Descriptors/MappingBuilderContext.cs 100.00% <ø> (ø)
...iptors/MappingBuilders/DictionaryMappingBuilder.cs 90.82% <ø> (ø)
.../MappingBuilders/DirectAssignmentMappingBuilder.cs 100.00% <ø> (ø)
.../Descriptors/MappingBuilders/EnumMappingBuilder.cs 85.28% <ø> (ø)
...tors/MappingBuilders/EnumToStringMappingBuilder.cs 100.00% <ø> (ø)
...iptors/MappingBuilders/EnumerableMappingBuilder.cs 95.00% <ø> (ø)
...escriptors/MappingBuilders/MemoryMappingBuilder.cs 88.46% <ø> (ø)
...uilders/NewInstanceObjectPropertyMappingBuilder.cs 91.17% <ø> (ø)
...criptors/MappingBuilders/NullableMappingBuilder.cs 92.85% <ø> (ø)
... and 47 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@latonz latonz marked this pull request as ready for review August 7, 2023 19:06
@latonz latonz requested a review from CommonGuy August 7, 2023 19:06
@latonz latonz merged commit 43e9b19 into riok:main Aug 8, 2023
12 of 14 checks passed
@latonz latonz deleted the expirement-full-name-of branch August 8, 2023 06:26
@github-actions
Copy link

github-actions bot commented Aug 8, 2023

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

The release is available on:

Your semantic-release bot 📦🚀

@TimothyMakkison
Copy link
Collaborator

Nice, I'll be interested to see what people think of this 😅

Out of curiosity, why did you move the parsing logic into AttributeDataAccessor.CreateMemberPath, didn't CallerArgumentExpressionAttribute work?

@latonz
Copy link
Contributor Author

latonz commented Aug 8, 2023

CallerArgumentExpressionAttribute does not work, because the Roslyn apis resolve only the compile type value provided to the constructor. Mapperly then constructs the PropertyMappingConfiguration type via reflection. Since it is initialized via reflection the CallerArgumentExpressionAttribute cannot be applied.

@github-actions
Copy link

🎉 This PR is included in version 3.1.0 🎉

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants