future: Convert subject -> topic in webapp/backend code.

[showell]

No description provided.

[timabbott]

What you're thinking around topic_name vs. topic as the name to use in the API? I suppose the idea is that topic_name is more extensible in case in the future we choose to add a topic_id field?

Because aside from that, I feel like from an API-readability standpoint, topic feels a lot cleaner (we can easily rename from topic -> topic_name for the local variable inside the view code with REQ, so what we want in our own code is sorta irrelevant).

[timabbott]

I'd do this as a new commit on top since the old code was wrong in the same way, but this function is incorrect; it should be subject__icontains=.

[showell]

I am not really sure why you think this should be "icontains". I added a comment to that effect for now. I can deal with this in the second round of edits if you give me a bit more context (and I may change the name of the function to something more like messages_containing_topic.

[timabbott]

Oh, I meant iexact.

[timabbott]

Generally looks pretty good; I posted a bunch of small comments.

[showell]

I'm changing "topic_name" to "topic" per your suggestion, then I'm going through the inline comments.

[showell]

This is ready for the second round of review. The first 20 commits were in your first review--I may add more later this morning that you haven't seen yet.

The only thing I have a question about is your subject__icontains comment related to the archive query. I just want confirmation before I fix that up (or just leave that out of the scope of this, since my fix preserves the current behavior).

[showell]

I swept the tests for "subject." I tried to be pretty thoughtful about not making tests "too easy to pass" if we change things. Most tests that use "subject" are just using it in passing. The tests that actually focus on the name "subject", whether it's an event or the DB model, are a bit vulnerable to false positives, but I think even most of those are basically valid if we do sane things during the migration.

[showell]

The last bunch of lint commits are to speed up lint, and I can probably move them to another PR if that helps. It was helpful to split out functions when I was profiling why it's slow, so that's why some commits just do extractions. The current bottlenecks is actual calls to re.search, which is good (what you'd expect) and bad (hard to make faster).

  8    ncalls  tottime  percall  cumtime  percall filename:lineno(function)
  9         1    0.012    0.012    5.962    5.962 custom_check.py:909(check_custom_checks_py)
 10       979    0.112    0.000    5.950    0.006 custom_check.py:149(custom_check_file)
 11     39394    1.671    0.000    5.230    0.000 custom_check.py:101(check_file_for_pattern)
 12   5386695    3.560    0.000    3.560    0.000 {method 'search' of '_sre.SRE_Pattern' objects}
 13       979    0.191    0.000    0.350    0.000 custom_check.py:68(get_line_info_from_file)
 14       979    0.070    0.000    0.108    0.000 custom_check.py:80(get_rules_applying_to_fn)
 15     39394    0.019    0.000    0.062    0.000 re.py:222(compile)
 16    279254    0.057    0.000    0.057    0.000 {method 'strip' of 'str' objects}
 17       979    0.033    0.000    0.050    0.000 custom_check.py:222(check_file_for_long_lines)

[timabbott]

@showell this is great. I merged a bunch of things, and then stopped in two places:

• For the linter, I want to keep the Python 2 compatibility code, because we're planning to extract the core linter code as a third-party project, and that project should support Python 2.
• For zerver/lib/api_test_helpers.py, if I'm not mistaken, that content appears in the API documentation on /api. I think we need to (A) make sure the python-zulip-api package handles this and/or (B) do a Zulip server release supporting the aliases feature.

(I think you should be able to easily rebase cleanly). Include Eeshan in any discussion on the API stuff.

[showell]

I fixed the linter. I kinda hate having that code just sitting there and bitrotting till we release our linter, but it's two lines, so whatever.

For the API stuff, I avoided touching the pieces that get excerpted to docs. If you look at the code and my diff, you'll see it's fairly easy to see what's safe.

[showell]

Here's the remaining use of "subject" in the API code (cause it's wrapped for doc magic):

    # {code_example|start}
    # Send a stream message
    request = {
        "type": "stream",
        "to": "Denmark",
        "subject": "Castle",
        "content": "I come not, friends, to steal away your hearts."
    }
    result = client.send_message(request)
# {code_example|end}

[showell]

I've added some JS-side cleanups as well. The last of those is probably the most non-trivial of them.

[timabbott]

FWIW, Zev and I did some work at a hackathon a couple months ago on extracting and releasing the linter, and probably the work could be finished in about a day. So it isn't a totally abstract concept.

[timabbott]

Makes sense. I merged all but the last 2 commits as the series ending in 6546fb3.

[timabbott]

For the compose things, what do you think about calling the HTML elements #compose_topic and #compose_stream? I feel like msg_ is a weird prefix for the compose area, since it sounds like it's an element within an individual message.

[timabbott]

(I merged the other commits on here).

[showell]

The first two commits here are ready to merge. They're both a bit more involved than many of the commits leading up to this that you already merged, @timabbott, but nothing too risky/controversial as far as I can tell.

[timabbott]

(Holding off on merging this until we get the compose_recipient_stream / compose_recipient_topic change done (or whatever we're calling it)).

[showell]

There's still a lot of work to do before we can merge the commit here, but the goal is to get here.

[zulipbot]

Heads up @showell, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.