Skip to content

Include mapping configuration #1833

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 59 commits into from
Jun 16, 2025
Merged

Include mapping configuration #1833

merged 59 commits into from
Jun 16, 2025

Conversation

faddiv
Copy link
Contributor

@faddiv faddiv commented May 11, 2025 •
edited
Loading

Include mapping configuration

Work in progress Info

Hi @latonz,

I decided to try implementing this feature because I feel this is an important feature, and we could also use it.
Since the basics of this started working as expected, and I found the integration points that feel correct, I think I can create a complete implementation.
Currently, it is still work in progress, but this way you are aware that I'm working on it and you can guide me.

Implementation Details

  • MembersMappingConfiguration is extended with an IncludedMapping property that holds the target mapping's name.
  • MembersMappingConfiguration is extended with a Name property that holds the current mapping's name.
  • MappingCollection now also holds the MappingBuilderContext in a ListDictionary. This is used to get context by name.
  • Created IncludeMappingBuilder which recursively walks through the mappings and connects the configurations.
  • Since I made this implementation recursive (Can go deeper than one level), I introduced a diagnostic for circular reference.
  • For not found mapping, I used ReferencedMappingNotFound. (Not sure if it is correct, or a new one is needed.)
  • Implemented NamedMapping. Currently, it is simply an alias, and it doesn't have preference over the unnamed mappings.
  • Documentation added.

Fixes #513

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)

faddiv
faddiv commented May 12, 2025
View reviewed changes
@latonz
Copy link
Contributor

latonz commented May 13, 2025

Thank you for this contribution — at first glance, it looks pretty good! 😊
Unfortunately, I don't currently have the time to review a PR this big. Hopefully, I will have the time this week or at the beginning of next week. Thank you for your understanding.

@faddiv
Copy link
Contributor Author

faddiv commented May 13, 2025

Thank you for this contribution — at first glance, it looks pretty good! 😊 Unfortunately, I don't currently have the time to review a PR this big. Hopefully, I will have the time this week or at the beginning of next week. Thank you for your understanding.

No problem, it is still a long way to go. 🙂
Currently, I have at most one hour daily to work on this anyway.

faddiv
faddiv commented May 15, 2025
View reviewed changes
@faddiv
Copy link
Contributor Author

faddiv commented May 17, 2025

Added the NamedMapping implementation too. Currently, it is just a simple alias and is treated as the default name. If duplicated with unnamed mapping it is reported.
Question: What should happen if a mapping has the same name as an unnamed mapping or another named mapping?
Also, I think this named mapping should be used by another setting that references methods by name. Like Use on MapProperty.

@faddiv faddiv force-pushed the feature/Include-mapping-configuration branch from 335fae0 to 32a8e5b Compare May 17, 2025 17:54
@faddiv
Copy link
Contributor Author

faddiv commented May 18, 2025

I saw that nested mappings processed separatelly. First I just tried to use recursive mapping there too. This lead to duplicate error reportings.
Now I created a separate class: IncludeMappingBuilder. This makes the connections and error reporting, and the generation parts go through the mapping contexts. Since I think the MappingBuilderContext is intended to be immutable, I kept it this way.

@faddiv faddiv force-pushed the feature/Include-mapping-configuration branch from 1c09496 to 2408297 Compare June 3, 2025 16:19
faddiv
faddiv commented Jun 3, 2025
View reviewed changes
Copy link
Contributor Author

@faddiv faddiv left a comment

Choose a reason for hiding this comment

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

I leave some remarks about my changes, so we can discuss them.

faddiv
faddiv commented Jun 5, 2025
View reviewed changes
@faddiv
Copy link
Contributor Author

faddiv commented Jun 8, 2025

I made a few improvements on the tests.
What do you think? When will you have time for the review? Currently, I have nothing more to improve on the implementation.

@latonz
Copy link
Contributor

latonz commented Jun 11, 2025

I'll have a look this week or the beginning of the next week. Thank you for your efforts!

latonz
latonz requested changes Jun 12, 2025
View reviewed changes
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 for all your work and your patience!

@faddiv
Copy link
Contributor Author

faddiv commented Jun 12, 2025

I think I managed to fix the test failures, but if there are still some, I'll take care of them another day with the rest of feedbacks.

@faddiv
Copy link
Contributor Author

faddiv commented Jun 12, 2025

Also, I realized, the .NET 6.0 integration tests don't run.

@faddiv
Copy link
Contributor Author

faddiv commented Jun 15, 2025 •
edited
Loading

Hopefully, I managed to fix all the build errors, except one:
Please add the enchantment label.

@latonz latonz added the enhancement New feature or request label Jun 16, 2025
@latonz latonz merged commit cae1e86 into riok:main Jun 16, 2025
28 of 31 checks passed
This was referenced Jun 16, 2025
Merged
Closed
Closed
@faddiv faddiv deleted the feature/Include-mapping-configuration branch July 14, 2025 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@latonz latonz latonz approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Include mapping configuration

2 participants

@faddiv @latonz