Skip to content

initial_snapshot: Add emailAddressVisibility override in InitialSnapshot method #1011

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

Merged
merged 1 commit into from
Nov 1, 2024
Merged

initial_snapshot: Add emailAddressVisibility override in InitialSnapshot method #1011

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/example_data.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,7 @@ InitialSnapshot initialSnapshot({
zulipMergeBase: zulipMergeBase ?? recentZulipVersion,
alertWords: alertWords ?? ['klaxon'],
customProfileFields: customProfileFields ?? [],
emailAddressVisibility: EmailAddressVisibility.everyone,
emailAddressVisibility: emailAddressVisibility ?? EmailAddressVisibility.everyone,
Copy link
Collaborator

Choose a reason for hiding this comment

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

initial_snapshot: Add emailAddressVisibility override in InitialSnapshot method

The InitialSnapshot method does not override the
emailAddressVisibility in the example_data.dart
with the value passed in the function arguments.
This commit fixes the bug

Commit message nit: I think the paragraph ("The InitialSnapshot method…") isn't necessary; it just repeats what's already clear in the code change. Instead of that paragraph, how about:

  • "It looks like this was left out by accident." (or something like that)
  • Say whether or not any callers were passing emailAddressVisibility. That helps make clear what changes this is making to our tests' behavior, if any. (From a quick search, I think none of the callers were passing it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks for the comment, I will update the commit message.

Yes, at the moment no callers were passing emailAddressVisibility, but when I was working on the @mention autocomplete items, I was trying to write a test that required passing the emailAddressVisibility, that is when I discovered the issue, Even if it is not used now, I think there will be future scenarios where it would be needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. Yes, this is still a good change to make! :) Just that it's helpful to know if it's a change that affects behavior of existing tests. It sounds like it doesn't, and that's helpful to write down in the commit message, for anyone who's reading it who doesn't know that yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please post on the PR thread when you've made the change and it's ready for another review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @chrisbobbe
I have made updates to the commit, please review.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! And one more thing I missed last time. In the commit message summary line—

initial_snapshot: Add emailAddressVisibility override in InitialSnapshot method
  • Keep it to 76 characters or shorter
  • Let's use "test:" for the prefix, so it's clear it's purely about test code
  • The method's name is initialSnapshot, not InitialSnapshot. Our convention is to import the example-data methods as eg., so eg.initialSnapshot is a compact way to identify the method that's nice and clear that it's an example-data method.

So, how about:

test: Fix eg.initialSnapshot to use emailAddressVisibility override

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thank you. Will update that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the commit message

serverTypingStartedExpiryPeriodMilliseconds:
serverTypingStartedExpiryPeriodMilliseconds ?? 15000,
serverTypingStoppedWaitPeriodMilliseconds:
Expand Down