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

zcommands: Add zcommand module and separate test module. #9758

Merged
merged 1 commit into from Jun 16, 2018
Merged

zcommands: Add zcommand module and separate test module. #9758

merged 1 commit into from Jun 16, 2018

Conversation

rheaparekh
Copy link
Collaborator

@rheaparekh rheaparekh commented Jun 15, 2018 •
edited

Move the zcommands from /views/messages.py to /lib/zcommand.
Also, move the zcommand tests from /tests/test_messages.py to /tests/test_zcommand.

I split this up from #9754 to make it an easy review.

@rheaparekh
zcommands: Add zcommand module and separate test module.
409da42 
Move the zcommands from '/views/messages.py' to
'/lib/zcommand'.

Also, move the zcommand tests from '/tests/test_messages.py'
to '/tests/test_zcommand'.
neiljp
neiljp reviewed Jun 16, 2018
View reviewed changes
Copy link
Contributor

@neiljp neiljp left a comment •
edited

Choose a reason for hiding this comment

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

I agree it is good to extract this code into a separate location, and the individual tests are definitely clearer. My only concern is it feels a little strange to just have a simple wrapper left in messages.py.

lonerz
lonerz reviewed Jun 16, 2018
View reviewed changes
Copy link
Member

@lonerz lonerz left a comment

Choose a reason for hiding this comment

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

I think this looks good, code-wise. I'm not sure what you would do to change the one-line function.

@neiljp
Copy link
Contributor

neiljp commented Jun 16, 2018

Agreed about the one-line function; I expect as the code develops further then common code could well appear there in any case.

@showell showell merged commit d0bc8d0 into zulip:master Jun 16, 2018
@rheaparekh rheaparekh deleted the zcommand branch June 17, 2018 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljp neiljp neiljp left review comments

@lonerz lonerz lonerz left review comments

Assignees
No one assigned
Labels
size: XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rheaparekh @neiljp @lonerz @showell @zulipbot