git-ubuntu

Merge ~bryce/git-ubuntu:project-c98-remove-cli-lint-command into git-ubuntu:master

  1. Git
  2. lp:~bryce/git-ubuntu
  3. project-c98-remove-cli-lint-command
  4. Merge into master
Proposed by Bryce Harrington
Status: Merged
Merge reported by: Bryce Harrington
Merged at revision: cae471875bb5601de4e32ecc9d325667de12cd52
Proposed branch: ~bryce/git-ubuntu:project-c98-remove-cli-lint-command
Merge into: git-ubuntu:master
Diff against target: 1105 lines (+110/-88) (has conflicts)
7 files modified
dev/null (+0/-72)
doc/gitubuntu-completion.sh (+0/-9)
gitubuntu/__main__.py (+4/-1)
gitubuntu/git_repository.py (+73/-1)
gitubuntu/git_repository_test.py (+32/-0)
gitubuntu/submit.py (+1/-1)
man/man1/git-ubuntu.1 (+0/-4)
Conflict in gitubuntu/git_repository.py
Reviewer Review Type Date Requested Status
Robie Basak Approve
Server Team CI bot continuous-integration Approve
Review via email: mp+384584@code.staging.launchpad.net

Description of the change

Drops lint and moves its remaining code into git_repository.

Also considered moving derive_target_branch() to submit, but I think it makes more sense for submit to depend on git_repository than vice versa, since submit more of a leaf type command. I'm a bit concerned this increases the size of the already big git_repository, but de-aggregating that module is a project for some other day.

I'm not sure I understand why _derive_target_branch_string() is split out separately from derive_target_branch(), but this is not an uncommon pattern in git_ubuntu, and the test case depends on being able to pass in a synthetic set of objects to the internal code, so I've kept it as-is. I'd like to understand the rationale for this approach since the wrapper function feels like it just adds an unnecessary extra layer.

I wouldn't mind doing some additional refactoring/cleanup on _derive_target_branch_string() so if you feel like suggesting some improvements please do. Otherwise, I can land it as is and leave it for future cleanup efforts.

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:cae471875bb5601de4e32ecc9d325667de12cd52
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/518/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/518//rebuild

review: Approve (continuous-integration)
Revision history for this message
Robie Basak (racb) wrote :

This branch seems to contain commits in its history that never made it into master in the form that they are here, so there are conflicts. But this can be fixed by just rebasing on to master - please do this before landing.

Apart from that, +1.

> I'm not sure I understand why _derive_target_branch_string() is split out separately from derive_target_branch(), but this is not an uncommon pattern in git_ubuntu, and the test case depends on being able to pass in a synthetic set of objects to the internal code, so I've kept it as-is. I'd like to understand the rationale for this approach since the wrapper function feels like it just adds an unnecessary extra layer.

> I wouldn't mind doing some additional refactoring/cleanup on _derive_target_branch_string() so if you feel like suggesting some improvements please do.

I think _derive_target_branch_string() is better tested in a way that doesn't entangle with a GitUbuntuRepository. I presume that's the logic behind keeping it separate. I accept though that a wrapper makes things hard to follow.

My preference would be to leave it as it is for now, but look at fixing this when we split out the GitUbuntuRepository megaclass. That way we might be able to resolve this directly without having to through the exercise of faking a GitUbuntuRepository - which IMHO would be worse than the wrapper.

review: Approve
Revision history for this message
Robie Basak (racb) wrote :

Perhaps push here for another CI run after the rebase and before landing please.

Revision history for this message
Bryce Harrington (bryce) wrote :

> Perhaps push here for another CI run after the rebase and before landing
> please.

Ah, sorry I pushed before I noticed this comment.
I did a rebase, then merge to master and verified only the 2 new commits came over. I think it should be good but will keep an eye on jenkins this evening.

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches