Skip to content
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

Jump to bottom

annotated zerver.lib.email_mirror #940

Closed
wants to merge 1 commit into from
Closed

annotated zerver.lib.email_mirror #940

wants to merge 1 commit into from

Conversation

medullaskyline
Copy link
Contributor

No description provided.

@smarx
Copy link

smarx commented Jun 5, 2016

Automated message from Dropbox CLA bot

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

@medullaskyline medullaskyline force-pushed the mypy-annotation-zerver.lib.email_mirror branch from c6f4e40 to 2847333 Compare June 5, 2016 19:57
timabbott
timabbott reviewed Jun 5, 2016
View reviewed changes
zerver/lib/email_mirror.py
import six

logger = logging.getLogger(__name__)

def redact_stream(error_message):
# type:(str) -> str
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

There should be a space between the : and ( in these annotations; can you fix that everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks. :)

@medullaskyline medullaskyline force-pushed the mypy-annotation-zerver.lib.email_mirror branch from 347b600 to 98943c5 Compare June 5, 2016 20:46
timabbott
timabbott reviewed Jun 5, 2016
View reviewed changes
zerver/lib/email_mirror.py
@@ -72,6 +79,7 @@ def get_missed_message_token_from_address(address):
return msg_string[2:]

def create_missed_message_address(user_profile, message):
# type: (UserProfile, UserMessage) -> str
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think message here is a Zulip Message from zerver.models (not a UserMessage)

@medullaskyline medullaskyline force-pushed the mypy-annotation-zerver.lib.email_mirror branch from 2bb76df to 429ee92 Compare June 5, 2016 21:55
@medullaskyline
Copy link
Contributor Author

Error message for the python 3.5 build:
zerver/lib/email_mirror.py:8: error: Module has no attribute 'message'
So it's one of those email issues I'm not sure how to resolve.

@sharmaeklavya2
Copy link
Member

Shouldn't you be using message.Message instead of message?
(https://docs.python.org/2/library/email.message.html#email.message.Message)

@sharmaeklavya2
Copy link
Member

The email issue is arising because typeshed is missing stubs for email.message in python 2.7. I have opened python/typeshed#275 for it.

Until the issue is resolved, you can silence this error somehow. Generally errors are silenced using # type: ignore, but you can use this hack instead:
Replace from email import message by import email.message as message.

This has the advantage that you won't have to update this line when stubs are added to typeshed.
@timabbott, is this good?

@timabbott
Copy link
Sponsor Member

@sharmaeklavya2 that hack makes sense to me.

@medullaskyline are you likely to have time to finish this PR up in the near future?

@medullaskyline
Copy link
Contributor Author

@timabbott Sure I can finish it tonight.

@medullaskyline medullaskyline force-pushed the mypy-annotation-zerver.lib.email_mirror branch from 38c3a5e to d999a5e Compare June 9, 2016 22:51
@medullaskyline
Copy link
Contributor Author

@timabbott @sharmaeklavya2 Looks like it's done, thanks for your help!

@sharmaeklavya2
Copy link
Member

sharmaeklavya2 commented Jun 9, 2016 •
edited

It looks like there are parameters in your pull request which should be annotated as text_type but they have instead been annotated as str. I recently had to bear a lot of trouble because some faulty annotations (which used str instead of text_type) were pushed into Zulip in the past. I'll get back to you with more details about what you should do after analyzing your code more carefully.

@sharmaeklavya2
Copy link
Member

It has been discussed here that it's better to do from six import text_type and use text_type instead of doing import six and using six.text_type. Can you make changes to your pull request accordingly?

sharmaeklavya2
sharmaeklavya2 reviewed Jun 11, 2016
View reviewed changes
zerver/lib/email_mirror.py
@@ -28,11 +32,13 @@ def redact_stream(error_message):
return error_message

def report_to_zulip(error_message):
# type: (str) -> None
Copy link
Member

Choose a reason for hiding this comment

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

error_message should be text_type instead of str.

@sharmaeklavya2
Copy link
Member

You should change most strings' type from str to text_type. Zulip's mypy docs have recently been updated and a section on annotating strings has been added to it. You might find that useful to decide between str and text_type.

@timabbott
Copy link
Sponsor Member

@medullaskyline just wanted to check in on whether you're planning to finish this?

@medullaskyline medullaskyline force-pushed the mypy-annotation-zerver.lib.email_mirror branch from ab92c88 to 55957a0 Compare June 18, 2016 03:32
sharmaeklavya2
sharmaeklavya2 reviewed Jun 20, 2016
View reviewed changes
zerver/lib/email_mirror.py
return 'missed_message:' + token


def is_missed_message_address(address):
# type: (str) -> bool
Copy link
Member

Choose a reason for hiding this comment

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

I believe address here is an email address. So it should be typed as text_type.

@sharmaeklavya2 sharmaeklavya2 mentioned this pull request Jun 20, 2016
Merged
@sharmaeklavya2
Copy link
Member

We have a commit message guideline. Something like Annotate zerver.lib.email_mirror. or Annotate zerver/lib/email_mirror.py. as the first line would be better.

@sharmaeklavya2
Copy link
Member

I think all strs in zerver/lib/email_mirror.py except the key type of debug_info should be text_type. I tried making that change to your pull request and I got a mypy error. But that is because there was a faulty annotation in zerver/lib/notifications.py (#1067). I got no errors after fixing that.

@timabbott
Copy link
Sponsor Member

Merged via #1073, thanks @medullaskyline for annotating this!

@timabbott timabbott closed this Jun 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@medullaskyline @smarx @sharmaeklavya2 @timabbott