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: Include all members of nested member #1245

Merged
merged 3 commits into from
Apr 29, 2024

Conversation

rhodon-jargon
Copy link
Contributor

@rhodon-jargon rhodon-jargon commented Apr 22, 2024

Flatten all nested members of a specific property

Description

The added MapNestedPropertiesAttribute allows users to flatten all properties of a specific member, to avoid having to manually specify the mapping of all nested properties of that member properties as shown in #453.

Partly based on #587, but re-implemented, keeping the comments on the PR into account. I took a different approach on the implementation, to explicitly handle nested properties differently from manually specified properties. The resulting member lookup priority is as follows:

  1. Manually specified property mappings using MapPropertyAttribute.
  2. Properties from the root object, which may or may not use automatic flattening.
  3. Properties from the objects at any of the specified MapNestedPropertiesAttributes, which may or may not use automatic flattening. Note that these objects are checked in no explicit order, though I suspect it will be based on the order the attributes are specified in. If multiple objects contain a member with the correct name, only the first is returned and no ambiguity diagnostic is emitted. This behavior can be changed in the future.

I thought this order would be most intuitive, and it also allows overriding the default behavior using MapperIgnoreSourceAttributes.

Fixes #453

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)

Copy link
Contributor

@latonz latonz left a comment

Choose a reason for hiding this comment

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

Thank you very much for these high-quality PRs 🥳
I added my feedback, feel free to discuss 😊

Since this is your first contribution to a riok project, I need to approve the CI runs manually...
Currently the integration tests for the older .NET versions are failing. You can find the VerifyTest diffs in the GH Actions artifacts (see also integration tests contributor documentation).

docs/docs/configuration/flattening.md Outdated Show resolved Hide resolved
docs/docs/configuration/flattening.md Outdated Show resolved Hide resolved
docs/docs/configuration/flattening.md Outdated Show resolved Hide resolved
src/Riok.Mapperly/Configuration/AttributeDataAccessor.cs Outdated Show resolved Hide resolved
test/Riok.Mapperly.IntegrationTests/Models/TestObject.cs Outdated Show resolved Hide resolved
@rhodon-jargon
Copy link
Contributor Author

rhodon-jargon commented Apr 29, 2024

Thank you very much for these high-quality PRs 🥳 I added my feedback, feel free to discuss 😊

Thank you for maintaining this! I've been using Mapster.Tool in the past, but that has a lot of problems and didn't work for .NET 8. Mapperly is a lot more intuitive, it just missed a few features that I needed :). I implemented all your feedback.

Currently the integration tests for the older .NET versions are failing.

Right, I forgot about the other .NET versions. Is there a way to run them all locally? I just updated the snapshots manually.

@latonz latonz enabled auto-merge (squash) April 29, 2024 13:56
@latonz latonz merged commit b69b2a9 into riok:main Apr 29, 2024
17 checks passed
@latonz
Copy link
Contributor

latonz commented Apr 29, 2024

Thank you for the updates 😊

I usually don't run the older TFM tests locally. It could work by adjusting the TargetFramework of the integration tests. However, I'm not sure if the older Roslyn version is used if the target framework is an older version.

Copy link

github-actions bot commented May 3, 2024

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

The release is available on:

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version 3.6.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.

Include all members from a nested member
2 participants