#6 - Added support for get_auth_backends, dev_get_emails and dev_fetch_api_key #11
Conversation
|
Hi! Thanks for the pull request. I unfortunately don't know enough about this codebase to review the code, but I can make a couple suggestions that will make it easier for other people to review and merge it! You can probably combine most of these commits into a single commit, to make it easier for others to review it. Here are some tips on tidying your commit history, including changing your commit messages. Thanks for writing descriptive commit messages. Here's a guide to help you write commit messages that are more consistent with our style, which makes it easier for us in the future -- when someone's trying to solve a new problem, if commit messages are all in the same format, it's easier to skim them in logs and quickly understand what's happening. Since this pull request does not completely fix issue #6, you don't have to use "Fixes: " but instead you can mention the issue number in the body of the commit message. And when you update your code or your commit message, you can update your pull request and the changes will appear here. :) Thanks. |
|
@brainwane : Thanks for the guidance. I will update the PR and refactor the commits (I think I have to start a new PR for that) whenever I get a chance. |
|
Hi! No, you won't have to start a new pull request; you can update this one. Please read https://zulip.readthedocs.io/en/latest/git-guide.html#update-a-pull-request for more. |
|
@erandiganepola When you refactor commits, you don't need to close the old PR and open a new one. You can update your pull request and we recommend you do that (instead of closing the old PR and opening a new one) because then it's easier to see the history of what you've done, to to check that you've responded to previous reviewers' comments, and to preserve links we've mentioned (in comments) with other issues and PRs. |
|
@brainwane : I see. But the problem is that my old commits (most are spacing and indentations fixes other than the first commit) will still be merged to main repo if I don't start a new PR. That's why I said, it is better if I submit a new PR. WDYT? |
|
@erandiganepola It's not going to be a problem because you're going to combine those commits into a single logical commit -- check out the links I provided, especially the one about tidying your commit history. If you still have questions about this, ask a question in the |
|
Thanks @brainwane ! I didn't know the concept of rebasing until today. Learned a lot today :) Hope the PR looks better now. |
…v_fetch_api_key
|
@erandiganepola I see that the automated tests have failed -- please see https://travis-ci.org/zulip/zulip-js/builds/215238937 for details so you can figure out what you need to fix. |
|
I will look into that and fix them. I couldn't find how to fix those failures yet. |
|
I think the Travis failures on this project might be happening for everyone. @arpith does the Travis CI setup on this project work? Looking at the code, I'm not sure how it could, since it seems to expect a Zulip development environment... |
|
Heads up @erandiganepola, 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 |
|
Sorry for not getting around to this PR in such a long time. Since there have been significant changes to the module since the PR was made, it would need significant refactoring. @erandiganepola are you interested in working on this? |
I have added support for 3(mentioned in topic) out of 5 required endpoints stated in #6. I have also added examples under /examples directory for those. I will later add the mocha tests for these functions as well.
It is better to wait till I implement and finalize the other two endpoints and tests in order to merge this PR.