Issue 1320

[Mannatgupta666]

Label current user as username"(you)" across DM-related views.

Here are screenshots showing the changes:

[AshutoshKhadse23]

@Mannatgupta666, refer https://zulip.readthedocs.io/en/latest/contributing/contributing.html
Also, make sure to use correct formatting, as in your pr, I can see the formatting changes too.
One more thing, CI failed you should either solve that.
And you have pushed unnecessary files too.

[Mannatgupta666]

Hi, thanks for the feedback.

I’ve gone through the contributing guide and fixed the issues you pointed out. I’ve removed the unintended formatting changes and reverted the (you) label from the message list so that all widget tests pass now. I’ve also removed the unnecessary files from the PR and re-ran CI locally to ensure everything is passing.

Please let me know if there’s anything else I should adjust. Thanks for the review!

[chrisbobbe]

What does the contributing guide say about commits and commit messages?

[Mannatgupta666]

Thanks for pointing that out! This is my first PR, and I missed the commit guide earlier. I’ve cleaned up my commits using git rebase -i so each is clear and coherent, and pushed an updated history. Commits should be minimal, self-contained, have clear messages, and pass tests (so any test updates needed by a change should be in the same commit as the original change).

CI is currently reporting analyzer warnings in lib/api/model/submessage.dart, which I didn’t modify in this PR. Could you please advise if this is a known issue on main, or how you’d like me to proceed in order to resolve the CL?

[gnprice]

The analyzer issue should be resolved by rebasing, now that #2091 is merged.

Before we can review this PR in detail, you will need to write tests for it:

• The new-DMs change has no test, and needs one.
• The recent-DMs change gets sort of accidentally tested by existing tests which you update… but the result is that it's not clear if we have any tests for the non-"you" case. Instead, there should be a test that's specifically about this behavior, and then the other tests should be adjusted so that they don't invoke this behavior.

I also see the formatting of the code is fairly messy. Please edit so that the new code matches the format of the existing code around it.

[Mannatgupta666]

Thanks for the detailed feedback. I’m working on the tests and code formatting now and will update the PR shortly.

[Mannatgupta666]

I’ve added the tests and fixed the formatting. The PR is ready for review whenever convenient.

[gnprice]

Thanks. This is still very messy, though, for example in the code highlighted below. That makes the code difficult to read.

Please look closely at where existing code does and doesn't use spaces to separate things, and at how it uses indentation, and then go carefully through your code and edit it to match.

[gnprice]

.

[Mannatgupta666]

Fixed the spacing and indentation. Please let me know if it looks better now or if there’s anything else I should adjust.

[gnprice]

That's improved. There's still ",children" missing a space, and the argument to Text needs indentation.

There's also the changes in the rest of the PR. This particular diff hunk was an example (and is the part you've edited in the current revision), but there are similar issues elsewhere.

[Mannatgupta666]

Fixed! Please check once.

[gnprice]

This revision fixed about half of the remaining spacing problems. You need to do this:

Please look closely at where existing code does and doesn't use spaces to separate things, and at how it uses indentation, and then go carefully through your code and edit it to match.

Read through your entire PR that way, line by line. It's not a good use of maintainers' time to review your work before you've carefully reviewed it yourself.

Meanwhile, zooming out: this doesn't handle all the changes requested in the issue. It also uses the old expect API in some of the new tests, which we don't use in any of our tests; see our README, or indeed any of our existing tests.