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.

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

boxes/helper: Updated to new quoting style. #854

Closed
wants to merge 3 commits into from

Conversation

zee-bit
Copy link
Member

@zee-bit zee-bit commented Dec 21, 2020

This PR aims to bring ZT one step more closer to its web counterpart. The quoting style is changed to include the name of the user whose message was quoted(and his user_id) along with a link that would narrow to the quoted message. This is the new markdown/bugdown syntax of a quoted message:

@**Full Name|1234(id)** said[link_to_msg]:
```quote
    quoted_msg
```

For interoperability, the link to the quoted message is the same as that of the web app, so narrowing to the quote in ZT is possible.

Also, the URL generation and encoding part of the code has been shifted to a new module named server_url.py to separate it from the rest of the code inside helper.py and make it easier for constructing and deconstructing server URL in the future.

This PR should fix #514 and #151(partially).

@zulipbot zulipbot added size: L [Automatic label added by zulipbot] enhancement New feature or request labels Dec 21, 2020
@zee-bit zee-bit force-pushed the issue_514 branch 2 times, most recently from 6555660 to ceb4711 Compare December 21, 2020 20:25
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@zee-bit Initial testing showed this to render fine, though possibly some topics with / in them don't work?

You have the updated quote style ok, but as we discussed in the stream the challenge is how to handle url generation. I'd suggest decoupling the message-to-url part (helper.py is fine for now) as you have done here (from the url-generation part, in another file). The server has url_encoding.py, but it uses other types connected to the server implementation like Realm and it uses f-strings (though we plan to to move to that soon with python3.6). My suggestion would be to use the server file as a basis for a module in our repo which handles chat server urls like this, and we can incorporate your and @preetmishra 's work if it builds upon that in a useful way.

Other than interoperability and tracking changes in the server, the reason I'd like this in a separate file is since we both construct and deconstruct server URLs from narrows and it'd be useful to have it together, and also is related to my hope that we could later integrate this into the python API package.

We can discuss more in the stream if you like.

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved
@neiljp neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Dec 23, 2020
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Dec 24, 2020
@zee-bit
Copy link
Member Author

zee-bit commented Dec 24, 2020

@zulipbot add "PR needs review"

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@zee-bit This is promising, but as per another PR we can tidy this up a little more in terms of commit structure :)

It'll be great to have the bugfix, but as with the other code I'd like to see text of how it was translated across from the server (if it was), and also some tests should help with indicating and checking behavior 👍

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved
zulipterminal/helper.py Outdated Show resolved Hide resolved
zulipterminal/server_url.py Outdated Show resolved Hide resolved
@neiljp neiljp removed the PR needs review PR requires feedback to proceed label Jan 6, 2021
@zee-bit
Copy link
Member Author

zee-bit commented Jan 7, 2021

@zulipbot add "PR needs review"
@zulipbot remove "PR needs update"

@neiljp neiljp added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jan 8, 2021
This commit is a bug-fix that corrects generation of fences for quoted
messages. The logic for generation of quote fence is borrowed from the
webapp. Fences are generated after searching for maximum length occurence
of ` through regex, and wrapping the quote block with the generated fence.
This allows for proper quoting of messages containing a code-block or
another quote inside them.
This is the first part of commit for this Pull-request.
Tests amended.
This is the second commit to this PR, which adds the same
quoting style as used in the webapp. The URL generation
part of the code now has its own module named server_url.py
along with the tests for this module added as test_server_url.py.
This will allow to track changes in the server and make constructing
and deconstructing server URL's easier. This module can also be later
integrated into the API package.

Tests amended.
Fixes zulip#514.
Adds the new module server_url.py along with its description in the
developer-documentation.
@zulipbot
Copy link
Member

Heads up @zee-bit, 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.

@neiljp
Copy link
Collaborator

neiljp commented Jan 20, 2021

@zee-bit I manually merged this just now with slightly adjusted commit text, those extra test cases I mentioned, and squashing the last two commits together. The last commit of the set is 7105240 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request has conflicts PR needs review PR requires feedback to proceed size: XL [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use new quoting style
4 participants