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

RUM-1613 Add mapper interface for traversing all children #1684

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

jonathanmos
Copy link
Member

@jonathanmos jonathanmos commented Oct 31, 2023

What does this PR do?

Add an internal interface for mappers that need to traverse all children during tree traversal.

Motivation

This interface is to be used by React Native for traversing RN view groups, as preparation for implementing Session Replay there.

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@jonathanmos jonathanmos force-pushed the jmoskovich/rum-1613/traversal-mapper branch 2 times, most recently from 0838ea9 to 74f29b3 Compare November 1, 2023 09:29
@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2023

Codecov Report

Merging #1684 (0ca6379) into develop (1fc5582) will increase coverage by 0.06%.
Report is 33 commits behind head on develop.
The diff coverage is 92.31%.

@@             Coverage Diff             @@
##           develop    #1684      +/-   ##
===========================================
+ Coverage    83.73%   83.79%   +0.06%     
===========================================
  Files          459      459              
  Lines        15778    15781       +3     
  Branches      2354     2355       +1     
===========================================
+ Hits         13211    13223      +12     
+ Misses        1938     1933       -5     
+ Partials       629      625       -4     
Files Coverage Δ
...ssionreplay/internal/recorder/TreeViewTraversal.kt 100.00% <100.00%> (ø)
...essionreplay/internal/recorder/SnapshotProducer.kt 87.88% <66.67%> (ø)

... and 16 files with indirect coverage changes

@jonathanmos jonathanmos marked this pull request as ready for review November 1, 2023 10:08
@jonathanmos jonathanmos requested a review from a team as a code owner November 1, 2023 10:08
@jonathanmos jonathanmos changed the base branch from louiszawadzki/poc-cross-platform-session-replay to develop November 2, 2023 09:46
@jonathanmos jonathanmos marked this pull request as draft November 2, 2023 09:47
@jonathanmos jonathanmos force-pushed the jmoskovich/rum-1613/traversal-mapper branch 2 times, most recently from 9c42246 to bd90c68 Compare November 2, 2023 18:55
@jonathanmos jonathanmos marked this pull request as ready for review November 2, 2023 19:34
Comment on lines 47 to 48
traversalStrategy = if (mapper is NonExhaustiveTypeMapper) {
TraversalStrategy.TRAVERSE_ALL_CHILDREN
Copy link
Member

Choose a reason for hiding this comment

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

Note

I'm confused about the naming of the interface: It says the mapper is Non exhaustive, but it would imply using a TRAVERSE_ALL_CHILDREN strategy. Can we be a bit more explicit about what is being non exhaustive here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Until now we've referred to mappers that return a node whenever they map to a specific class as being "exhaustive", and so the naming follows from this. How about "TraverseAllChildrenMapper"?

@jonathanmos jonathanmos force-pushed the jmoskovich/rum-1613/traversal-mapper branch 3 times, most recently from a839f65 to 0ca6379 Compare November 7, 2023 08:54
@jonathanmos jonathanmos force-pushed the jmoskovich/rum-1613/traversal-mapper branch from 0ca6379 to 8147727 Compare November 7, 2023 14:02
@jonathanmos jonathanmos merged commit 4db01f1 into develop Nov 7, 2023
@jonathanmos jonathanmos deleted the jmoskovich/rum-1613/traversal-mapper branch November 7, 2023 14:48
@xgouchet xgouchet added this to the 2.3.0 milestone Dec 13, 2023
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.

5 participants