CSC495-2014 / TeamworkEnglewoodGit

GNU General Public License v2.0
5 stars 15 forks source link

Null parameter checking, gitwrapper fix, and a comment fix #144

Closed samuelfrench closed 10 years ago

samuelfrench commented 10 years ago

Hi,

Here is what I have been working on. If anyone is testing this on windows remember to hardcode the git executable in the gitcommands model, and also you need to generate an ssh key.

works

(Now let's hope it passes the travis build... :P )

@cdwainscott

samuelfrench commented 10 years ago

Alright it doesn't like my switch statement...lemme fix

samuelfrench commented 10 years ago

Ok... I was testing the wrong copy... leave this open but give me a second to take a look

samuelfrench commented 10 years ago

@cdwainscott Ok, please take a look. I think it's good to go at this point.

mikeholler commented 10 years ago

Remember to only do composer install, not composer update. I'll accept all changes from this except for the composer.lock file. Please git checkout 7a67fdb8 -- composer.lock, then add and commit the new composer.lock file and push it up after making the other changes I requested.

samuelfrench commented 10 years ago

I'm on it.

On 3/15/2014 5:52 PM, Michael Holler wrote:

Remember to only do |composer install|, not composer update. I'll accept all changes from this except for the |composer.lock| file. Please |git checkout 7a67fdb8 -- composer.lock|, then add and commit the new |composer.lock| file and push it up after making the other changes I requested.

— Reply to this email directly or view it on GitHub https://github.com/CSC495-2014/TeamworkEnglewoodGit/pull/144#issuecomment-37740481.

samuelfrench commented 10 years ago

@apotheos Ok, I got the files up. It took me forever to figure out how to get my git repo up to date with the class one again (don't ask me why), so I'm going to take a break before I look into testing the changes I made. They are very minor though.

cdwainscott commented 10 years ago

Sam if you have time tomorrow we can get together and finalize testing your code. I should be in the lab most of the afternoon. Anyone else that wants to join is welcome.

samuelfrench commented 10 years ago

Around what time were you thinking? @cdwainscott

mikeholler commented 10 years ago

This looks good @samuelfrench! If you can just fix the switch statement that I commented on I'll merge this right away!

cdwainscott commented 10 years ago

@samuelfrench I am currently in the lab right now and plan on being here the whole day.

samuelfrench commented 10 years ago

Ok, is it OK if I come around 4:30-5?

On 3/16/14, 12:48 PM, David Wainscott wrote:

@samuelfrench https://github.com/samuelfrench I am currently in the lab right now and plan on being here the whole day.

— Reply to this email directly or view it on GitHub https://github.com/CSC495-2014/TeamworkEnglewoodGit/pull/144#issuecomment-37764298.

samuelfrench commented 10 years ago

@cdwainscott - do you have the project all setup in WAMP stack, or should I copy the vm I have it working on to my laptop before I come?

On 3/16/14, 12:48 PM, David Wainscott wrote:

@samuelfrench https://github.com/samuelfrench I am currently in the lab right now and plan on being here the whole day.

— Reply to this email directly or view it on GitHub https://github.com/CSC495-2014/TeamworkEnglewoodGit/pull/144#issuecomment-37764298.

cdwainscott commented 10 years ago

I have it working finally but im still running it on windows

samuelfrench commented 10 years ago

@apotheos Changes made. It actually looks 100x more readable, I don't know what I was thinking originally. I guess I'm just deathly afraid of using an if more than once consecutively.

samuelfrench commented 10 years ago

@apotheos @cdwainscott

Simplified check route params into one function.

cdwainscott commented 10 years ago

looks good

mikeholler commented 10 years ago

Last one, I promise. There are two routes in routes.php that go to the same controller function. Can you remove one of them? Doesn't matter which. The web interface will be able to handle either one without modification.

Route::post('/user/{user}/project/{project}/git-cmd', 'GitController@cmd');
Route::post('/user/{user}/project/{project}/git', 'GitController@cmd');
samuelfrench commented 10 years ago

No problem, I appreciate your assistance.

samuelfrench commented 10 years ago

Not sure why that was in there...I don't think I was the one who added it, but it doesn't matter as long as it's fixed.

Anything else for me to do? @apotheos

@cdwainscott

mikeholler commented 10 years ago

I put the original one there because you didn't have it in your very first pull request. No worries. Looks good!

mikeholler commented 10 years ago

You didn't call checkRouteParams correctly anywhere. Needs to be called like $this->checkRouteParams($user, $project);. Did you run this code?

image

mikeholler commented 10 years ago

@samuelfrench I fixed the problem with calling checkRouteParams and opened up PR #157. Sign off if you agree with the fix.

samuelfrench commented 10 years ago

Done.