search

EOL / capstone_eol

Capstone2011 fork or eol code
MIT License
1 stars 1 forks source link

refactoring mobile controllers #3

Closed yloginov closed 12 years ago

lwalley commented 13 years ago

The refactoring is great, it's really going to help us when it comes to maintaining or expanding the code in the future. The use of REST methods is just what we need. I did notice that the /spec/integration/mobile_spec.rb file has a conditional in it which means it will only run the mobile tests if a variable is set to true in an environments config file. I set this variable to true in my local.rb file and ran the /spec/integration/mobile_spec.rb test on its own and found a couple of broken tests related to the _path method name changes. Can you take a look?

yloginov commented 12 years ago

Hi Lisa, i fixed the test that was causing the problem. had to update the test to reflect the new path values. i merged the code to the master branch, hopefully i did it right. Could you please confirm?

On Sun, Sep 25, 2011 at 11:31 AM, lwalley < reply@reply.github.com>wrote:

The refactoring is great, it's really going to help us when it comes to maintaining or expanding the code in the future. The use of REST methods is just what we need. I did notice that the /spec/integration/mobile_spec.rb file has a conditional in it which means it will only run the mobile tests if a variable is set to true in an environments config file. I set this variable to true in my local.rb file and ran the /spec/integration/mobile_spec.rb test on its own and found a couple of broken tests related to the _path method name changes. Can you take a look?

Reply to this email directly or view it on GitHub:

https://github.com/EncyclopediaOfLife/capstone_eol/pull/3#issuecomment-2191459

lwalley commented 12 years ago

Yuri, I have merged your branch into the EOL master code (https://github.com/EncyclopediaOfLife/eol/commit/33a3fb451af00e84bc93038e77cc22e99abd9ff1). There were still some broken spec related to paths which I fixed please review the mobile spec file in my second commit to master: https://github.com/EncyclopediaOfLife/eol/commit/76516d716f7bcee80e29991b6ebdf51a3799bd88#diff-9. Were you able to run the spec tests locally? Were all mobile spec tests passing before you made your commit?

You'll notice in my second commit to EOL master (https://github.com/EncyclopediaOfLife/eol/commit/76516d716f7bcee80e29991b6ebdf51a3799bd88) that I made some other minor code changes. Some of these changes were related to the controller refactoring but others were just basic code cleanup not related to your work. Please review these changes and let me know if you have any questions about the changes I made.

With regards to merging your branch to master please review the code review process in the Wiki document http://wiki.eol.org/display/dev/Code+review+for+open+source+contributors. My understanding from that is that you should not need to merge your branch with master as you do not have write access to the EOL master. Instead you just make commits to your branch (which you created from the EOL master) and then when you make the pull request someone with write access will handle the merge with the EOL master, they will then push your commits to GitHub and once they have done that you can pull those changes down to your read version of EOL master. Then you can remove your old branch and create a new branch for your next task. Let me know if the process is unclear, I will ask Dima to review this comment in case I have misunderstood the documentation.

lwalley commented 12 years ago

I briefly spoke with Dima and he confirmed that whilst you should not need to merge your branch TO master, you may want to update your branch with code FROM master before making the pull request. This will make the merge easier for the people handling the pull request.

When I merged your pull request and then pushed the code to master the commit (https://github.com/EncyclopediaOfLife/eol/commit/33a3fb451af00e84bc93038e77cc22e99abd9ff1) went is as authored by me. I'm not sure why this happened as it should have had your name. Sorry about that, Dima and I are going to take a look and see if we can figure out why this occurred so that the work you do in the future will be properly attributed to you.

lwalley commented 12 years ago

Dima and I had a look and first of all, you'll be glad to know that your first commit back in September did get attributed to you in the EOL repository commit log: https://github.com/EncyclopediaOfLife/eol/commit/816e5d674723f0992bd329980fef8f70804a8747

Unfortunately with regards to your recent code changes it looks like you actually did not make any commit to your branch in GitHub. If you look at the commit log for your branch on GitHub: https://github.com/EncyclopediaOfLife/capstone_eol/commits/refactoring_controllers you will see the last commit is from September 24, 2011. That is why when I did the merge the specs were still broken.

We looked at the capstone_eol/master branch and saw that you had merged your refactoring_controller branch into the capstone_eol master "Merge pull request #3 from EncyclopediaOfLife/refactoring_controllers" and in that merge we saw the comment "changed the spec file to work with new path. added rake 0.8.7 to Gemfile". However, when we did a diff between that merge commit and your previous commit on September 24 there were no differences. So whatever code changes you made since September 24, these are not stored anywhere in GitHub. It may be that you made some changes locally that you did not push to the branch. If this is the case you can push your local changes to the branch and make a new pull request for me to review.

There are a couple of things you should note from this:

  1. You should not merge your branch into the capstone_eol/master. If you did not do this through the command line then possibly this was triggered through the pull request interface on GitHub. Just be careful when you make the pull request that all you do is "send pull request". Do not "merge the pull request" and do not "close the pull request".
  2. Make sure that before you send a pull request that you review the commit log for your branch on GitHub making sure the code changes you expect to be there are there.

Take a look at http://help.github.com/send-pull-requests/ note that you should follow the instructions for the following sections of the document: "Initiating The Pull Request" "Previewing The Pull Request" "Changing The Commit Range and Destination Repository" (you should not need to modify the range but check you have the right branch) "Sending The Pull Request"

That is as much as you need to do. You can also review "Managing Pull Requests" for further info how to find pull requests on GitHub, but do not proceed with any of the instructions after that, merging and closing pull requests for EOL will be managed by myself or Dima.

yloginov commented 12 years ago

sorry about merging with capstone_eol, the reason i did that was because i made code changes on U of T computer but when I got home i could not clone the repository from eol master because then i would lose my changes that i made during the code sprint. again sotty for any confusion and inconvinience this may have cause.

yloginov commented 12 years ago

that is really weird that you did not see my changes to the spec file. I also had an issue with rake 0.9.2 so i locked it down to 0.8.7 in the Gemfile. not sure if you guys have seen this before, but my rake commands fail when I use 0.9.2