Page MenuHomePhabricator

[lib] Add `avatar` to `ThreadChanges`
AbandonedPublic

Authored by atul on Apr 5 2023, 11:52 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jun 13, 11:40 PM
Unknown Object (File)
May 30 2024, 12:37 PM
Unknown Object (File)
May 25 2024, 4:39 AM
Unknown Object (File)
May 25 2024, 4:39 AM
Unknown Object (File)
May 13 2024, 7:14 AM
Unknown Object (File)
May 5 2024, 10:30 PM
Unknown Object (File)
May 5 2024, 5:43 AM
Unknown Object (File)
May 3 2024, 4:10 PM
Subscribers

Details

Reviewers
ashoat
ginsu
Summary

Add avatar field to ThreadChanges so we can call changeThreadSettings with an edit avatar request.

Next diff will handle validation on the keyserver

Test Plan

Will by tested implicitly by subsequent diffs.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.Apr 5 2023, 11:57 AM
ashoat requested changes to this revision.Apr 5 2023, 12:38 PM

You have a pattern of submitting tiny diffs... I find these hard to review because I don't see the context of your changes. Can you please include the actual changes instead of submitting a diff that only changes types?

This revision now requires changes to proceed.Apr 5 2023, 12:38 PM

Can you please include the actual changes instead of submitting a diff that only changes types?

Included changes in D7332. Combining these changes made my workflow more difficult:

057544.png (304×1 px, 378 KB)

Personally would prefer to be able to get these scaffolding type diffs out of the way so I don't need to constantly reposition things.

You have a pattern of submitting tiny diffs... I find these hard to review because I don't see the context of your changes.

I'm not sure what's hard to review here?

In D7320#218118, @atul wrote:

Can you please include the actual changes instead of submitting a diff that only changes types?

Included changes in D7332. Combining these changes made my workflow more difficult:

057544.png (304×1 px, 378 KB)

Personally would prefer to be able to get these scaffolding type diffs out of the way so I don't need to constantly reposition things.

Let's talk through this workflow in our next 1:1. It's not clear to me what you're doing with these "DO NOT LAND" diffs... I've personally never found it necessary to make commits like that, but perhaps there is a good reason it's necessary in this case.

It's also not clear to me how the "DO NOT LAND" diffs complicate my request to squash Flow-only diffs into the diffs that already have changes, but perhaps I'm looking at the wrong thing in your screenshot?

You have a pattern of submitting tiny diffs... I find these hard to review because I don't see the context of your changes.

I'm not sure what's hard to review here?

I don't have prior context on how the ThreadChanges type is used in the codebase. To review this diff, I first had to search for what that is. After that, I had to ask myself "are these all the changes that are necessary for your work?", which is pretty much impossible to answer without the context of what the rest of your work looks like.

This by itself is not crazy, but it's part of a pattern of submitting tiny diffs for review that contributes to making it hard to review your work. Unless there's a strong reason, I don't think we should separate type changes like this from the related code changes.