Refactor DataContext#1038
Conversation
…outside of mapping.
BenjaminRodenberg
added
the
maintainability
uekerman
left a comment
uekerman
left a comment
There was a problem hiding this comment.
Good effort to bring more structure into DataContext.
I am, however, not sure whether the solution to have a third field participantData is the best way to go. Before, there were two data fields, which isn't ideal already. Now, we solve this by adding a third field 😁
If we stick with this solution (I see the advantages as well), we need to add more documentation and find a more intuitive name than "participantData".
|
Should be merged first: |
Co-authored-by: Frédéric Simonis <[email protected]>
uekerman
left a comment
uekerman
left a comment
There was a problem hiding this comment.
Looks good. One minor comment below.
Last thing: now that we moved functionality into DataContext it could be helpful for the long run to unit-test this separated functionality, right?
|
I added two tests for the mapping context. There are two minor todos that I will take a look at later, but the test is generally ready for review. |
|
Tests look good |
2 participants
Main changes of this PR
Turns
DataContextinto a class. Add corresponding getters, setters and restrict access. Add memberparticipantDataand only settoDataandfromData, when a mapping exists.Motivation and additional information
This allows us to control access on
toDataandfromData. We will be able to add further functionality to theDataContextthat might be required for implementation of waveform relaxation (#1029).Author's checklist
docs/changelog/if there are noteworthy changes. --> Not relevant, since refactoring.tools/formatting/check-formatand everything is formatted correctly.Reviewers' checklist