zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

Published 20 hours ago • verified publisher icon zulip

Show tutorial at start.

Open amanagr opened this issue 5 years ago • 5 comments

@neiljp if you think this PR is easier to review, then I will push the changes in the other PR. I did a complete change of commits. Let me know your thoughts on this.

amanagr avatar Aug 11 '19 10:08 amanagr

@neiljp do the splits look fine? I will proceed to adding tests then...

amanagr avatar Aug 16 '19 07:08 amanagr

zt_tutorial Apart from the new isort rule errors, everything works fine. @neiljp @sumanthvrao Let me know your thoughts on this.

amanagr avatar Mar 07 '20 12:03 amanagr

Thanks @kaustubh-nair for your review, let me get back to you with what I think of them.

amanagr avatar Mar 19 '20 13:03 amanagr

@amanagr You have one commit that removes a lot, and then another commit that adds a lot of features; could you combine them into a 'transition' commit? Otherwise, doesn't this lead to a broken build between commits? Even if that isn't considered as big a deal, it's more difficult to understand what the change is, as it's over two commits?

Also, the tests you remove in one commit - isn't that functionality that we still want to include (failure to load) ?

neiljp avatar Mar 22 '20 04:03 neiljp

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

zulipbot avatar Apr 17 '20 00:04 zulipbot