#845 Feature for issue #806 - Commit comparison
Closed by ogutierrez. Opened by ogutierrez.
ogutierrez/pagure wip/commitcompare  into  master

Download 845.patch
no initial comment

Where would branchname come from?

I was wondering if we could reuse the pull_request template here instead of having this large if block in the repo_info.

I didn't investigate, just thought :)

I've been slowly moving from assertTrue to assertIn but it's up to you which one you like best.

We don't really need a branch do we?

branchname and branch: Oops. I removed the branch things but left that behind. Will remove it in a fix commit.

The assertIn, ok. Will change it too. It seems more accurate :)

Will investigate the pull_request template to check if could be used instead that if block

Pull-Request has been updated

Finally I used the pull request template as you said. It was easier to adapt it than I imagined. :)

I suspect this will not please pep8, could you see about splitting this over 2 lines?

(Similar for the lines below when they are more than 80 characters)

So I've come to realize that having URLs of type / isn't a good idea, especially if we want to work on things like namespace of pseudo-namespace.

Since this is a new URL endpoint, let's try to get it right from the start, I propose:
/<repo>/c/<commit1>..<commit2>/

Thoughts?

Sorry for the late review, I had missed your last comment, but kudos, looks much better this way :)

Pull-Request has been updated

The PEP8 styling has been done. I've the line length limit warning disabled in the editor and forgot to check it.

I agree with the /<repo>/c/<commit1>..<commit2>/ schema. Will change it as soon as you confirm.

Thanks :)

I agree with the //c/../ schema. Will change it as soon as you confirm.

I confirm :)
(Even more, check the namespace branch ;-))

Have checked it, but don't know if you want me to do something with that. I've seen you reworked a lot of routes as well as some of the views.

Will push the route modifications and wait for your response.

Pull-Request has been updated

Oh, since this is a new endpoint, no need to enforce a size for the commit hash.

I did this in namespace to keep backward compatibility, but if you check the endpoint for /<repo>/v/<commit> it doesn't enforce a commit length, allowing to use shorter hash.

Ok. Removing! :)

Pull-Request has been updated

Looks good to me.

Could you rebase your branch? I'll test the change locally and if all goes well merge :)

Pull-Request has been closed by ogutierrez

Metadata