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.

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

restore text_* #777

Merged
merged 2 commits into from Aug 7, 2017

Conversation

Projects
None yet
4 participants
@Eldinnie
Member

Eldinnie commented Aug 7, 2017

This restores the text_* properties for message to the way they were.
Implementing different methods for different behavior can be done either in this PR or in another.

fixes #773

Eldinnie added some commits Aug 7, 2017

@Eldinnie

This comment has been minimized.

Show comment
Hide comment
@Eldinnie

Eldinnie Aug 7, 2017

Member

I modified as requested.
properties now available are:
text_html <- does not encode MessageEntity.URL
text_html_urled < encodes MessageEntity.URL
text_markdown <- does not encode MessageEntity.URL
text_markdown_urled < encodes MessageEntity.URL

Member

Eldinnie commented Aug 7, 2017

I modified as requested.
properties now available are:
text_html <- does not encode MessageEntity.URL
text_html_urled < encodes MessageEntity.URL
text_markdown <- does not encode MessageEntity.URL
text_markdown_urled < encodes MessageEntity.URL

@jsmnbom

This comment has been minimized.

Show comment
Hide comment
@jsmnbom

jsmnbom Aug 7, 2017

Member

Hmm maybe add a docstring link from the urled one to the not urled one and vise versa?
Other than that LGTM

Member

jsmnbom commented Aug 7, 2017

Hmm maybe add a docstring link from the urled one to the not urled one and vise versa?
Other than that LGTM

@Eldinnie

This comment has been minimized.

Show comment
Hide comment
@Eldinnie

Eldinnie Aug 7, 2017

Member

@bomjacob they should be listed right on top of each other in docstrings. I think it's a bit overkill

Member

Eldinnie commented Aug 7, 2017

@bomjacob they should be listed right on top of each other in docstrings. I think it's a bit overkill

@jsmnbom

This comment has been minimized.

Show comment
Hide comment
@jsmnbom

jsmnbom Aug 7, 2017

Member

That may be true, but I still think it makes more sense from a usability standpoint. Especially if you consider that people don't necessarily search through the docs themselves, they might be linked by roolsbot or be using the internal help() command.

Member

jsmnbom commented Aug 7, 2017

That may be true, but I still think it makes more sense from a usability standpoint. Especially if you consider that people don't necessarily search through the docs themselves, they might be linked by roolsbot or be using the internal help() command.

@91DarioDev

This comment has been minimized.

Show comment
Hide comment
@91DarioDev

91DarioDev Aug 7, 2017

@bomjacob
about this in your comment:
text_html <- does not encode MessageEntity.URL

does not encode if the user didn't use html tags, but does it returns the html tags if the user sent the message using them?

91DarioDev commented Aug 7, 2017

@bomjacob
about this in your comment:
text_html <- does not encode MessageEntity.URL

does not encode if the user didn't use html tags, but does it returns the html tags if the user sent the message using them?

@Eldinnie

This comment has been minimized.

Show comment
Hide comment
@Eldinnie

Eldinnie Aug 7, 2017

Member

@91DarioDev in that case it would be a MessageEntity.TEXT_LINK which is always encoded

Member

Eldinnie commented Aug 7, 2017

@91DarioDev in that case it would be a MessageEntity.TEXT_LINK which is always encoded

@tsnoam

This comment has been minimized.

Show comment
Hide comment
@tsnoam

tsnoam Aug 7, 2017

Member

LGTM. @Eldinnie has proper unitest wip with the new unitests so no need for that here.

Member

tsnoam commented Aug 7, 2017

LGTM. @Eldinnie has proper unitest wip with the new unitests so no need for that here.

@tsnoam tsnoam merged commit 70057a6 into master Aug 7, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 86.305%
Details

@tsnoam tsnoam deleted the rollback_text_ branch Aug 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment