search

kahmali / meteor-restivus

REST APIs for the Best of Us! - A Meteor 0.9+ package for building REST APIs https://atmospherejs.com/nimble/restivus
MIT License
544 stars 116 forks source link

Add login result to data.extra #168

Closed aramk closed 8 years ago

kahmali commented 8 years ago

The modification looks good. If you add some tests to check for the presence/absence of the data.extra field in some responses, and add it to the documentation, I'll pull this in.

kahmali commented 8 years ago

Also, thanks so much for submitting this PR!! I'm in such a rush I almost forgot that very important bit :)

kahmali commented 8 years ago

Any update on this? Another user was requesting the functionality in #194. I imagine you've just been busy.

aramk commented 8 years ago

Sorry about that, I've added tests now

kahmali commented 8 years ago

Oh, awesome! Did not expect that quick of a response. I'll take a look at the tests, and if they look good and everything passes I'll merge this in tonight and publish a new version. I'll keep you posted. Thanks again for putting this together!

aramk commented 8 years ago

:+1: no worries, thanks for building this great library.

kahmali commented 8 years ago

Okay, the tests look good and they're all passing. Awesome! I'll consider creating an explicit test for the data.extra field (instead of testing for it throughout other tests), but this is enough to verify the functionality for now. I'll merge this in soon, then I just need to update the docs (to remove the "coming soon" label from this feature) and the change log before deploying. Thanks again for the quick response!

kahmali commented 8 years ago

Sorry, I spoke to soon. You'll need to rebase this onto devel and do a force push to this branch (otherwise you'll have to create a new PR and we'll lose this comment history). Sorry about that. I was looking at GitHub claiming that there were no conflicts with the base branch up above.

EDIT: To force push to a branch you can just run git push origin <your_branch_name> --force after rebasing. That will overwrite whatever is on origin on that branch.

kahmali commented 8 years ago

If it's not too late, while you're rebasing, feel free to squash these into a single commit. No worries if you don't. It's fine as two commits. Just not totally necessary, since we never really want features to exist without accompanying tests, so the intermediate commit is kind of a "dead" commit. Like I said though, no big deal if you've already rebased.

aramk commented 8 years ago

The squash and rebase is done, but I've never used it in my workflow before. I favour keeping the history intact as it helps with identifying in which commit a bug was introduced, as well as keeping the dates intact for record keeping.

kahmali commented 8 years ago

Not sure what you mean about keeping the dates intact. You'll see that the date of the original commit is intact. Git keeps record of the original date a commit was authored and the date of the most recent modification. Just run git cat-file -p <commit_sha> to see the detailed commit info (notice the difference between the author and commit dates):

$ git cat-file -p dace68ce8d42efa0a
tree a6e22145a03440454890ae2897983ad1f9e64ec0
parent 7876327dbbb0ddcab60066a0b45c9c43c34ca167
author Aram Kocharyan <akarmenia@gmail.com> 1453036205 +1100
committer Aram Kocharyan <akarmenia@gmail.com> 1457752390 +1100

Add login result to data.extra

Add tests for extra field

I always love to learn about different git workflows (even though I believe I have found the perfect one :P ), but like I said, that intermediate commit was "bad" history, as it contains untested code. Ideally, tests should be written before any code is changed. If the commit with the tests came first I would have said to just leave it. Squashing it seemed easier than asking you to swap their order. Just trying to keep up with the conventions I've done my best to follow in this repo. I don't care about having ALL the history, so much as having a clean history. Just a difference in philosophy is all. Definitely not claiming my way is better.

Anywho, I'll get this merged in ASAP. Just finishing up something for another project and then I'll take care of it. You'll get a notification as soon as it's merged and closed. I'll make those updates to the docs and publish the new version shortly after that (should be v0.8.9). Once again, thanks for the PR and the feedback. Greatly appreciated!

kahmali commented 8 years ago

This was released in v0.8.9. Thanks for all your help @aramk!

aramk commented 8 years ago

Cheers for the merge and explanation :+1: