channel: Rename StreamStore to ChannelStore

[Khader-1]

Fixes parts of #631

[gnprice]

Thanks @Khader-1! Excited to see this happening.

On a quick skim, the one high-level thing I notice is that this version changes what it expects from the API — the latter three commits all change the name we expect some properties to have in the JSON we get from the API, in addition to changing the name we use for those properties within our own code. I expect this means that the app won't work at this version.

As #631 says, we shouldn't be trying to change what we expect from the API at this stage. See there for a discussion of how to avoid such a change while updating our codebase's internal names.

This change should also have tests, though the tests will be pretty simple. See dd78af1 and other commits in #751 for examples of tests for similar renames.

Once you fix things so that the app works with these changes, and the changes have tests, this looks like it'll be ready for review. So at that point please go ahead and mark it for review: hit the "Ready for review" button so it's no longer a draft, give it the "buddy review" label, and request a reviewer.

[Khader-1]

Thanks @gnprice! I see this can easily grow out of hand! What would you suggest to tackle it? I think having a dedicated PR for every model/feature would help.

[gnprice]

Yeah, breaking this up into a handful of PRs would be good.

I think a key thing for making this clean to read is: when you rename a type, try to follow up immediately in the next few commits by renaming the fields, the local variables, and the parameters that have that type.

For example, after renaming StreamStore and StreamStoreImpl, ideally the next commits right after that would be the ones renaming fields, local variables, and parameters with those types. In this branch, those changes are separated by a number of other changes which rename various things within StreamStore and StreamStoreImpl. And there are a number of places remaining in the code that still have the "stream" names for such values, which causes them to look like:

    required ChannelStore streamStore,

As a reader that looks puzzling and rather less reassuring than either

    required StreamStore streamStore,

or

    required ChannelStore channelStore,

would be.

Or put another way: try to group the changes by the data being renamed, more than by the file or directory that the rename is happening in.

The type ZulipStream/Channel itself might be a bit of an exception to this, in that there are so very many places it appears. I think it'll probably be easiest and cleanest if that type is handled last, so that everything that's about other types (channel narrows, the channel store, channel events, etc.) is already saying "channel".

Then probably a good way to group the changes into PRs would be by those types:

• one PR for StreamStore/ChannelStore and its implementation, along with the values of those types
• one PR for the narrow types
• etc.

[Khader-1]

Thanks @gnprice! That's really helpful. I do have a question though. When it comes to renaming references of a code object, for example:

When renaming ChannelStore.streams to ChannelStore.channels, one reference for store.streams / store.channel is:

/// Parse the operand of a `stream` operator, returning a stream ID.
///
/// The ID might point to a stream that's hidden from our user (perhaps
/// doesn't exist). If so, most likely the user doesn't have permission to
/// see the stream's existence -- like with a guest user for any stream
/// they're not in, or any non-admin with a private stream they're not in.
/// Could be that whoever wrote the link just made something up.
///
/// Returns null if the operand has an unexpected shape, or has the old shape
/// (stream name but no ID) and we don't know of a stream by the given name.
int? _parseStreamOperand(String operand, StreamStore store) {
  // "New" (2018) format: ${stream_id}-${stream_name} .
  final match = RegExp(r'^(\d+)(?:-.*)?$').firstMatch(operand);
  final newFormatStreamId = (match != null) ? int.parse(match.group(1)!, radix: 10) : null;
  if (newFormatStreamId != null && store.streams.containsKey(newFormatStreamId)) {
    return newFormatStreamId;
  }

  // Old format: just stream name.  This case is relevant indefinitely,
  // so that links in old conversations continue to work.
  final String? streamName = decodeHashComponent(operand);
  if (streamName == null) return null;
  final stream = store.streamsByName[streamName];
  if (stream != null) return stream.streamId;

  if (newFormatStreamId != null) {
    // Neither format found a stream, so it's hidden or doesn't exist. But
    // at least we have a stream ID; give that to the caller.
    return newFormatStreamId;
  }

  // Unexpected shape, or the old shape and we don't know of a stream with
  // the given name.
  return null;
}

As you can see, there are local variables, docs, comments and the method name itself referring to "channel" as "stream". When I tried to change them, I felt like I was altering things beyond the scope of the commit. What would you suggest in such cases?

[gnprice]

Yeah, one key bit in my recommendation above is that those followup renames happen in the next few commits — not in the same commit.

In any case, what I'd suggest for the next single PR at this stage is:

one PR for StreamStore/ChannelStore and its implementation, along with the values of those types

I.e. rename StreamStore, and StreamStoreImpl, and the values that are a StreamStore or StreamStoreImpl.

In the example function above, I think the only thing that touches is the type of the parameter, which becomes ChannelStore.

Then another PR doing that for StreamNarrow.

Another PR can do that for StreamEvent and its subclasses.

And so on for one type or cluster of types at a time, but leaving ZulipStream for the end. You can get a rough list of types with a command like this:

$ git grep -howP '([A-Z]\w*)?Stream\w*' | sort | uniq -c | sort -rn
     75 StreamNarrow
     66 StreamColorSwatch
     50 ZulipStream
     34 StreamMessage
     32 StreamColorSwatches
     27 StreamColorVariant
     25 UnreadStreamSnapshot
     20 Stream
     15 FcmMessageStreamRecipient
[…]

Then at the end handle ZulipStream itself, and all the things that are a stream or a stream ID. That's most of what appears in that example function. That last wave of renames might be more organized by file instead of by type of data.

[sm-sayedi]

Thanks @Khader-1. LGTM!
I think it will be enough to add the "Fixes parts of..." label just in one commit.

Moving on to the mentor review. cc @kenclary

[gnprice]

Thanks @Khader-1, and thanks @sm-sayedi for the review!

For the remaining PRs within #631, let's use an abbreviated process where we go straight from buddy review to integration review, skipping mentor review and maintainer review. On the one hand there'll generally be less to review in them; and on the other, this sort of cross-codebase refactor is especially prone to developing conflicts, so it's especially good to get them merged soon after they're sent.

I squashed the five commits down into two, so these:
ed47a91 channel [nfc]: Rename file from "stream" to "channel"
ee8da94 channel [nfc]: Rename StreamStore to ChannelStore
b2edc4a channel [nfc]: Rename references of ChannelStore to channelStore
95ff1e0 channel [nfc]: Rename StreamStoreImpl to ChannelStoreImpl
df88682 channel [nfc]: Rename references of ChannelStoreImpl to match it's name

became:
1e979a3 channel [nfc]: Rename StreamStore/Impl to ChannelStore/Impl, and rename file
99ed080 channel [nfc]: Rename references of ChannelStore/Impl to match new names

because that grouping felt natural, and the commits remain pretty small and easy to read.

It's fine that the original commits were a bit more fine-grained than I wanted them to end up, though. In general it's a lot easier for me to squash than to split up, so that's the right direction to err in.

I also applied @sm-sayedi's comment, putting the reference in the first commit (since that's now clearly the main commit, and the second commit is a followup).

With those changes, this looks good — so I'm going ahead and merging. Thanks again!