Annotation of zerver/views/* and zerver/models.py #759

ashishk1994 · 2016-05-06T16:31:58Z

No description provided.

smarx · 2016-05-06T16:32:00Z

Automated message from Dropbox CLA bot

@ashishk1994, it looks like you've already signed the Dropbox CLA. Thanks!

timabbott · 2016-05-06T20:42:19Z

zerver/views/report.py

@@ -72,7 +79,8 @@ def json_report_error(request, user_profile, message=REQ, stacktrace=REQ,
                      ui_message=REQ(validator=check_bool), user_agent=REQ,
                      href=REQ, log=REQ,
                      more_info=REQ(validator=check_dict([]), default=None)):
-
+    # type: (Any, UserProfile, Any, Any, REQ, Any, Any, Any, REQ) -> HttpResponse
+    # TODO: INCOMPATIBLE_TYPES_WITH_REQ_TYPE_OBJECT_AND_REQ


If we can't figure out another solution, we could just get rid of the no-argument REQ() function model.

(zulip-venv) tabbott@zaset:~/zulip$ git grep '=REQ[,)]' zerver/decorator.py: def _wrapped_view_func(request, api_key=REQ, zerver/decorator.py: def _wrapped_view_func(request, email=REQ, api_key=REQ('api_key', default=None), zerver/views/__init__.py:def json_invite_users(request, user_profile, invitee_emails=REQ): zerver/views/__init__.py:def api_fetch_api_key(request, username=REQ, password=REQ): zerver/views/__init__.py:def update_active_status_backend(request, user_profile, status=REQ, zerver/views/__init__.py:def json_refer_friend(request, user_profile, email=REQ): zerver/views/__init__.py:def add_apns_device_token(request, user_profile, token=REQ, appid=REQ(default=settings.ZULIP_IOS_APP_ID)): zerver/views/__init__.py:def add_android_reg_id(request, user_profile, token=REQ): zerver/views/__init__.py:def remove_apns_device_token(request, user_profile, token=REQ): zerver/views/__init__.py:def remove_android_reg_id(request, user_profile, token=REQ): zerver/views/messages.py:def render_message_backend(request, user_profile, content=REQ): zerver/views/report.py:def json_report_error(request, user_profile, message=REQ, stacktrace=REQ, zerver/views/report.py: ui_message=REQ(validator=check_bool), user_agent=REQ, zerver/views/report.py: href=REQ, log=REQ, zerver/views/streams.py:def add_default_stream(request, user_profile, stream_name=REQ): zerver/views/streams.py:def remove_default_stream(request, user_profile, stream_name=REQ): zerver/views/streams.py:def json_rename_stream(request, user_profile, old_name=REQ, new_name=REQ): zerver/views/streams.py:def json_make_stream_public(request, user_profile, stream_name=REQ): zerver/views/streams.py:def json_make_stream_private(request, user_profile, stream_name=REQ): zerver/views/streams.py:def json_stream_exists(request, user_profile, stream=REQ, zerver/views/user_settings.py: full_name=REQ, zerver/views/users.py:def add_bot_backend(request, user_profile, full_name=REQ, short_name=REQ, zerver/views/users.py:def create_user_backend(request, user_profile, email=REQ, password=REQ, zerver/views/users.py: full_name=REQ, short_name=REQ): zerver/views/webhooks/github.py:def api_github_landing(request, user_profile, event=REQ, zilencer/views.py:def report_error(request, deployment, type=REQ, report=REQ(validator=check_dict([]))):

The other thing that's kinda not ideal about the way REQ is interacting with mypy for the views is that ideally, we'd be able to have mypy be checking the things we actually care about, namely the types of the arguments to the function constructed by the decorators here.

E.g. inside the body of the json_report_error function here, ui_message is a bool and ideally the type checker would know that...

Ok,

So we should modify it as REQ() from REQ as annotating argument with REQ considers it as REQ() type.

Yes, Ideally we should annotate the argument with bool or its known type.. but whenever I tried to do that It gave me the similar error..
Incompatible types in assignment (expression has type "REQ", variable has type "bool")

Yeah. Let me look into what the options are for annotating REQ. For now, I think probably you should focus your efforts on non-view code until we figure out the right way to handle this...

timabbott · 2016-05-12T22:00:06Z

Adding mention of python/mypy#1503 since that has some ideas for improving this

ashishk1994 · 2016-05-17T17:46:01Z

@timabbott Can you please review my models.py and stream file.

timabbott · 2016-05-18T00:02:28Z

zerver/models.py

@@ -93,6 +101,7 @@ def completely_open(domain):
    return not realm.invite_required and not realm.restricted_to_domain

 def get_unique_open_realm():
+    # type: () -> Realm


This should probably return an Optional[Realm], since the return value might be None, right?

The same comment probably applies to a bunch of other functions in this PR; assuming you agree these should hvae Optional in the return value, can you review everything that might return None in here and fix them all @ashishk1994 ?

timabbott · 2016-05-18T00:13:30Z

@ashishk1994 thanks for working on this! I posted some comments. You should rebase this onto master when you fix the various issues. It'd be great to at least get the models.py piece finished and merged quickly so we can avoid more merge conflicts.

For the views code, I think you should stop writing new annotations for new views files and instead focus on figuring out a model where we can specify the actual types of the arguments to the view functions in the annotations. Have you have any luck trying to get Guido's ideas for annotating REQ properly to work?

Also @sharmaeklavya2 FYI to avoid duplication of work.

sharmaeklavya2 · 2016-05-19T08:25:06Z

@ashishk1994, I think you should change the title to mention that you're also annotating zerver/models.py.

sharmaeklavya2 · 2016-05-20T03:28:33Z

zerver/models.py

@@ -63,6 +69,7 @@ def flush_per_request_caches():
 @cache_with_key(lambda *args: display_recipient_cache_key(args[0]),
                timeout=3600*24*7)
 def get_display_recipient_remote_cache(recipient_id, recipient_type, recipient_type_id):
+    # type: (int, int, int) -> List[Dict[str, Any]]


The return type should be something like Union[str, List[Dict[str, Any]]] because Line 80 returns a string.

sharmaeklavya2 · 2016-05-20T03:55:15Z

I realized that when using --py2, we can't pass a unicode where a str is expected, but we can pass a str where a unicode is expected. @ashishk1994, your code uses str everywhere. I think we should be careful about this. I have been using six.text_type to annotate parameters where a unicode might be passed.

@timabbott, What are your thoughts on this? How have you been handling strings?

sharmaeklavya2 · 2016-05-20T13:11:46Z

zerver/models.py

    try:
        return get_stream_backend(stream_name, realm)
    except Stream.DoesNotExist:
        return None

 def bulk_get_streams(realm, stream_names):
+    # type: (Realm, STREAM_NAMES) -> Dict[Any, Any]


Even though STREAM_NAMES = TypeVar('STREAM_NAMES', List[str], Set[str]) would work fine as the type of stream_names, I think something like Intersection[Sized, Iterable[str]] would make more sense. Unfortunately, that's not yet implemented in mypy (see python/typing#213).

I'm adding this comment to document the fact that we need this feature in a real-life scenario.

I think until then it'll be better to use TypeVar('STREAM_NAMES', Sequence[str], AbstractSet[str]) instead of TypeVar('STREAM_NAMES', List[str], Set[str])

sharmaeklavya2 · 2016-05-22T07:14:21Z

@ashishk1994 you should update your PR by rebasing it on master (There have been some changes since then about annotations and mypy version has also been updated). That way I can easily see your changes and check for any conflicts between our PRs (I'm annotating zerver/lib/actions.py #815).

Since Travis currently does not fail on wrong annotations (see #814), you might want to add some files to the exclude list in tools/run-mypy so that tools/run-mypy doesn't list those files every time you run it.

sharmaeklavya2 · 2016-05-22T07:24:22Z

https://groups.google.com/forum/#!topic/zulip-devel/hPJ5tBXPyIs (Use ABCs as type of parameters while type annotating code)

timabbott · 2016-05-29T11:59:38Z

@ashishk1994 can you rebase onto master?

Also, I get a bunch of mypy errors when I run it on this now that #815 has been merged. I think most of them are because you're using str rather than six.text_type which we've been getting via from six import text_type in the annotations for functions like get_stream in models.py.

If you can clean those up, I'd love to get the models.py commit merged since it's a relatively active file; meanwhile I'll plan to work with the mypy developers next week on the REQ issue; hopefully we can come up with a satisfactory solution.

timabbott · 2016-05-31T03:07:26Z

@ashishk1994 also I think we will end up needing to eliminate the use of just REQ (as opposed to REQ()) in order to make the approach implemented in 8c6afac work properly. Might make sense to just do that migration in its own commit...

timabbott · 2016-05-31T14:31:45Z

(I just fixed the REQ with no arguments issue in 960144a)

…ache.py. Previously it was int while call from models.py expect it to be str so this commit defines it as Any.

timabbott · 2016-06-03T14:39:18Z

I cleaned up and merged the models.py annotations, since they are important for the other people who are working on annotations to build on during the PyCon sprints; you'll probably have some merge conflicts since I changed a few things.

timabbott · 2016-06-08T00:11:11Z

Closing this PR, since I think all of these files were fully annotated during the Pycon sprints last week. Thanks @ashishk1994 for your work on this, especially the models.py work and discovering the issue with REQ (which was critical to our having a good solution for annotating views files during the sprints).

timabbott reviewed May 6, 2016
View reviewed changes

ashishk1994 force-pushed the Annotation branch from 0eee4d5 to fddeea2 Compare May 8, 2016 14:24

ashishk1994 force-pushed the Annotation branch 4 times, most recently from 164b3e8 to b17abbb Compare May 15, 2016 11:55

timabbott reviewed May 18, 2016
View reviewed changes

ashishk1994 changed the title Annotation of zerver/views/* Annotation of zerver/views/* and zerver/models.py May 19, 2016

sharmaeklavya2 reviewed May 20, 2016
View reviewed changes

ashishk1994 force-pushed the Annotation branch 4 times, most recently from 1461236 to b00b85f Compare May 22, 2016 06:19

ashishk1994 force-pushed the Annotation branch 3 times, most recently from 7160d27 to 4219a67 Compare May 23, 2016 17:15

ashishk1994 force-pushed the Annotation branch 3 times, most recently from 32279e4 to 118fdac Compare May 28, 2016 12:06

timabbott mentioned this pull request May 30, 2016

Annotation of zerver/views/* and zerver/models.py #845

Closed

ashishk1994 added 5 commits June 2, 2016 12:10

Type annotation of zerver/views/alert_words.

11ae21d

Type annotation of zerver/views/report.py.

e150759

Correction in annotation of generic_bulk_cached_fetch in zerver/lib/c…

6e795f0

…ache.py. Previously it was int while call from models.py expect it to be str so this commit defines it as Any.

Type annotation of zerver/models.py

4328ec5

Annotation of ~/zerver/views/streams.py

f54e8ed

ashishk1994 force-pushed the Annotation branch from 118fdac to f54e8ed Compare June 2, 2016 06:48

timabbott closed this Jun 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotation of zerver/views/* and zerver/models.py #759

Annotation of zerver/views/* and zerver/models.py #759

ashishk1994 commented May 6, 2016

smarx commented May 6, 2016

timabbott May 6, 2016

timabbott May 6, 2016

ashishk1994 May 7, 2016

timabbott May 7, 2016

timabbott commented May 12, 2016

ashishk1994 commented May 17, 2016 •

edited

Loading

timabbott May 18, 2016

timabbott May 18, 2016

timabbott commented May 18, 2016

sharmaeklavya2 commented May 19, 2016

sharmaeklavya2 May 20, 2016

sharmaeklavya2 commented May 20, 2016

sharmaeklavya2 May 20, 2016

sharmaeklavya2 May 21, 2016

sharmaeklavya2 commented May 22, 2016 •

edited

Loading

sharmaeklavya2 commented May 22, 2016

timabbott commented May 29, 2016 •

edited

Loading

timabbott commented May 31, 2016

timabbott commented May 31, 2016 •

edited

Loading

timabbott commented Jun 3, 2016

timabbott commented Jun 8, 2016

Annotation of zerver/views/* and zerver/models.py #759

Annotation of zerver/views/* and zerver/models.py #759

Conversation

ashishk1994 commented May 6, 2016

smarx commented May 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented May 12, 2016

ashishk1994 commented May 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented May 18, 2016

sharmaeklavya2 commented May 19, 2016

Choose a reason for hiding this comment

sharmaeklavya2 commented May 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharmaeklavya2 commented May 22, 2016 • edited Loading

sharmaeklavya2 commented May 22, 2016

timabbott commented May 29, 2016 • edited Loading

timabbott commented May 31, 2016

timabbott commented May 31, 2016 • edited Loading

timabbott commented Jun 3, 2016

timabbott commented Jun 8, 2016

ashishk1994 commented May 17, 2016 •

edited

Loading

sharmaeklavya2 commented May 22, 2016 •

edited

Loading

timabbott commented May 29, 2016 •

edited

Loading

timabbott commented May 31, 2016 •

edited

Loading