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.

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

Merge TimelineItemEventRow's textForInReplyTo and attachmentThumbnailInfoForInReplyTo functions #1859

Merged
merged 7 commits into from
Nov 27, 2023
Merged

Merge TimelineItemEventRow's textForInReplyTo and attachmentThumbnailInfoForInReplyTo functions #1859

merged 7 commits into from
Nov 27, 2023

Conversation

julioromano
Copy link

@julioromano julioromano commented Nov 22, 2023 •
edited
Loading

The flow is somewhat misleading so its logic has been merged into InReplyToDetails.metadata().

@julioromano julioromano self-assigned this Nov 22, 2023
@julioromano julioromano force-pushed the julioromano/polls_refactor branch from 02377c1 to df92530 Compare November 22, 2023 15:13
Base automatically changed from julioromano/polls_reply_fix to develop November 22, 2023 15:14
@julioromano julioromano force-pushed the julioromano/polls_refactor branch from df92530 to 1436208 Compare November 22, 2023 15:15
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 22, 2023 •
edited
Loading

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/DXaJVW

@codecov Codecov
Copy link

codecov bot commented Nov 22, 2023 •
edited
Loading

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (b8436fc) 66.44% compared to head (07a71a8) 66.48%.

Files Patch % Lines
.../messages/impl/timeline/model/InReplyToMetadata.kt 90.47% 0 Missing and 4 partials ⚠️
...s/impl/timeline/components/TimelineItemEventRow.kt 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1859      +/-   ##
===========================================
+ Coverage    66.44%   66.48%   +0.03%     
===========================================
  Files         1308     1308              
  Lines        33059    33055       -4     
  Branches      7087     7083       -4     
===========================================
+ Hits         21966    21976      +10     
+ Misses        7733     7724       -9     
+ Partials      3360     3355       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@julioromano julioromano force-pushed the julioromano/polls_refactor branch from 1436208 to 859b1f5 Compare November 24, 2023 15:01
@julioromano julioromano changed the title Remove TimelineItemEventRow.textForInReplyTo(inReplyTo: InReplyTo.Ready): String function Merge TimelineItemEventRow's textForInReplyTo and attachmentThumbnailInfoForInReplyTo functions Nov 24, 2023
@julioromano julioromano marked this pull request as ready for review November 24, 2023 15:02
@julioromano julioromano requested a review from a team as a code owner November 24, 2023 15:02
@julioromano julioromano force-pushed the julioromano/polls_refactor branch 2 times, most recently from f570640 to b8bb431 Compare November 24, 2023 15:33
jmartinesp
jmartinesp approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

LGTM, with a comment.

...kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt Outdated
is PollContent -> eventContent.question
else -> ""
}
private sealed interface InReplyToMetadata {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really know if it actually makes a difference, but using @Immutable with sealed interfaces should help in avoiding unnecessary recompositions, since otherwise the compose compiler doesn't know how to treat the interface AFAIK.

Copy link
Author

Choose a reason for hiding this comment

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

Should I add it to the sealed interface or to its subclasses?

Copy link
Member

Choose a reason for hiding this comment

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

To the sealed interface.

@jmartinesp
Copy link
Member

jmartinesp commented Nov 24, 2023 •
edited
Loading

Could we also add some mapping tests for InReplyToMetadata? I know they usually wouldn't be worth it, but given our coverage objective we should probably add those so coverage doesn't decrease.

@julioromano julioromano force-pushed the julioromano/polls_refactor branch 2 times, most recently from 3dc4f94 to 3f0c431 Compare November 27, 2023 16:50
@julioromano julioromano added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Nov 27, 2023
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Nov 27, 2023
@julioromano
Copy link
Author

Could we also add some mapping tests for InReplyToMetadata? I know they usually wouldn't be worth it, but given our coverage objective we should probably add those so coverage doesn't decrease.

All done! Might wanna take another look as the tests ended up being quite verbose.

@julioromano julioromano enabled auto-merge (squash) November 27, 2023 16:53
@julioromano julioromano added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Nov 27, 2023
@github-actions github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Nov 27, 2023
@julioromano julioromano added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Nov 27, 2023
@julioromano julioromano disabled auto-merge November 27, 2023 21:06
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Nov 27, 2023
@julioromano julioromano added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Nov 27, 2023
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Nov 27, 2023
Marco Romano added 2 commits November 27, 2023 22:11
Merge TimelineItemEventRow's textForInReplyTo and attachmentThumbnail…
6a633f6 
…InfoForInReplyTo functions

The flow is somewhat misleading so its logic has been merged into `InReplyToDetails.metadata()`.
Add @immutable to sealed interface
c09e5d2
@julioromano julioromano force-pushed the julioromano/polls_refactor branch from 2f622f3 to d32bc5c Compare November 27, 2023 21:11
@julioromano julioromano added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Nov 27, 2023
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Nov 27, 2023
@julioromano julioromano enabled auto-merge (squash) November 27, 2023 21:56
@sonarqubecloud SonarQubeCloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@julioromano julioromano disabled auto-merge November 27, 2023 22:02
@julioromano julioromano enabled auto-merge (squash) November 27, 2023 22:07
@julioromano julioromano merged commit 7bdb310 into develop Nov 27, 2023
@julioromano julioromano deleted the julioromano/polls_refactor branch November 27, 2023 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmartinesp jmartinesp jmartinesp approved these changes

Assignees

@julioromano julioromano

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@julioromano @jmartinesp