Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Api blob contents #1157

Merged
merged 4 commits into from Aug 1, 2012
Merged

Api blob contents #1157

merged 4 commits into from Aug 1, 2012

Conversation

punkisdead
Copy link

I added a couple of methods to the projects API.

  • Get the details about a single branch
  • Retrieve the raw contents for a file

Let me know what you think.

Regards,
Jeremy Anderson

@NARKOZ
Copy link
Contributor

NARKOZ commented Jul 27, 2012

Thanks. I'll give it a closer look after this weekend.

Please rebase it against master. Why Gemfile is changed?

@punkisdead
Copy link
Author

I needed to make sure grape was using at least version 0.2.1 because of the introduction of the "content_type" function. Then I also ran into a problem with the newer version of rake, and per a suggestion from @dhh himself on twitter, I rolled it back to 0.8.7.

I'll rebase it against master tonight.

--Jeremy

On Jul 27, 2012, at 5:30 PM, Nihad Abbasov reply@reply.github.com wrote:

Thanks. I'll give it a closer look after this weekend.

Please rebase it against master. Why Gemfile is changed?


Reply to this email directly or view it on GitHub:
#1157 (comment)

@punkisdead
Copy link
Author

I've pulled the changes from upstream and rebased this branch against master.

@dzaporozhets
Copy link
Member

@punkisdead Thank you for PR. @NARKOZ will review it but I have 2 problems with:

  1. Gemfile.lock has a lot of changes. We should not make gem updates & feature in one PR. Please rollback gems updates.
  2. Can you provide a real reason why Rake was downgraded, please?

@punkisdead
Copy link
Author

I had some error messages that led me to this post by Dave Chelimsky (http://blog.davidchelimsky.net/2011/05/28/rake-09-and-gem-version-constraints/). I know it's older, but it's what fixed my issue. The Grape API gem needs to be at least 0.2.1 as one of the functions I wrote requires the "content_type" function as I mentioned earlier, so I can change the mime-type of the response.

@dzaporozhets
Copy link
Member

I really have no time to read. Can you explain in few words? Cause I still dont understand how rake 09 can cause problems in your PR feature

@@ -1,6 +1,7 @@
module Gitlab
# Projects API
class Projects < Grape::API

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

@NARKOZ
Copy link
Contributor

NARKOZ commented Jul 30, 2012

@CodeAdept what version of ruby are you using? What error message do you get without rake 0.8.7? I'm sure this issue isn't related to Gitlab.

Run bundle update grape to update only grape.

@punkisdead
Copy link
Author

I'm not sure what happened, but Rake 0.9.2 is working for me now. I didn't update any other dependencies except for that and Grape, all of the rest of the updates to the Gemfile.lock are probably because I'm building this on a fresh install and there are newer versions of all of the gems since you've been developing this project. All I did was run bundle install after checking out the project and those were the versions it gave me.

I've made the other changes as requested.

# GET /projects/:id/repository/branches/:branch_id
get ":id/repository/branches/:branch_id" do
@project = current_user.projects.find_by_code(params[:id])
@branch = @project.repo.heads.find { |item| item.name == params[:branch_id] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@punkisdead you forgot to update these lines 😄

@NARKOZ
Copy link
Contributor

NARKOZ commented Jul 30, 2012

bundle install shouldn't change Gemfile.lock. Can you revert Gemfile.lock changes and try to update only grape?

Please don't forget to update doc/api/projects.md and squash your commits.

error!('404 Commit Not Found', 404) unless commit

tree = Tree.new commit.tree, user_project, ref, params[:filepath]
error!('404 File Not Found', 404) unless tree && tree.tree
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless tree.try(:tree)

@ghost ghost assigned NARKOZ Jul 30, 2012
@punkisdead
Copy link
Author

What do you mean by squash my commits?

@dzaporozhets
Copy link
Member

  1. revert Gemfile.lock except grape
  2. Squash commtis http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html
  3. Create a good commit description
  4. Apply @NARKOZ notes

@punkisdead
Copy link
Author

Ok, thanks for being patient guys. It's been a while since I've done any Rails development, and the first time "forking" and submitting a pull request on Github.

@dzaporozhets
Copy link
Member

@punkisdead hey congrats. Thank you for being patient also. Just tell us when you ready for final review :)

@punkisdead
Copy link
Author

Ok, commits should be squashed now and docs updated.

NARKOZ added a commit that referenced this pull request Aug 1, 2012
@NARKOZ NARKOZ merged commit d63706d into gitlabhq:master Aug 1, 2012
@NARKOZ
Copy link
Contributor

NARKOZ commented Aug 1, 2012

Nice. Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants