launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/code/browser/branchmergeproposal.py'
> --- lib/lp/code/browser/branchmergeproposal.py	2015-06-10 10:25:28 +0000
> +++ lib/lp/code/browser/branchmergeproposal.py	2015-06-12 02:31:22 +0000
> @@ -1011,6 +1011,7 @@
>                  'prerequisite_git_path',
>                  ]
>          field_names.extend([
> +            'commit_message',
>              'description',
>              'break_link',
>              ])

+register-merge has description before commit_message, and it is probably good to be consistent here.

> @@ -1044,7 +1045,8 @@
>                      merge_prerequisite = None
>              proposal = self.context.resubmit(
>                  self.user, merge_source, merge_target, merge_prerequisite,
> -                data['description'], data['break_link'])
> +                data['commit_message'], data['description'],
> +                data['break_link'])
>          except BranchMergeProposalExists as e:
>              message = structured(
>                  'Cannot resubmit because <a href="%(url)s">a similar merge'
> 
> === modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
> --- lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-06-05 17:49:22 +0000
> +++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2015-06-12 02:31:22 +0000
> @@ -925,6 +925,7 @@
>          new_proposal = view.resubmit_action.success(self._getFormValues(
>              context.merge_source, context.merge_target,
>              context.merge_prerequisite, {
> +                'commit_message': None,
>                  'description': None,
>                  'break_link': False,
>                  }))
> @@ -943,6 +944,7 @@
>              view.context.merge_source)
>          new_proposal = view.resubmit_action.success(self._getFormValues(
>              new_source, new_target, new_prerequisite, {
> +                'commit_message': 'a commit',
>                  'description': 'description',
>                  'break_link': False,
>                  }))
> @@ -964,6 +966,7 @@
>              merge_prerequisite = context.merge_prerequisite
>          return view.resubmit_action.success(cls._getFormValues(
>              context.merge_source, context.merge_target, merge_prerequisite, {
> +                'commit_message': None,
>                  'description': None,
>                  'break_link': break_link,
>                  }))
> @@ -1108,9 +1111,11 @@
>          """Proposals can be resubmitted using the browser."""
>          bmp = self.factory.makeBranchMergeProposalForGit(registrant=self.user)
>          browser = self.getViewBrowser(bmp, '+resubmit')
> +        browser.getControl('Commit Message').value = 'dribble'
>          browser.getControl('Description').value = 'flibble'
>          browser.getControl('Resubmit').click()
>          with person_logged_in(self.user):
> +            self.assertEqual('dribble', bmp.superseded_by.commit_message)
>              self.assertEqual('flibble', bmp.superseded_by.description)
>  
>  
> 
> === modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
> --- lib/lp/code/interfaces/branchmergeproposal.py	2015-06-10 10:25:28 +0000
> +++ lib/lp/code/interfaces/branchmergeproposal.py	2015-06-12 02:31:22 +0000
> @@ -598,6 +598,8 @@
>              proposal (defaults to the current merge_prerequisite).
>          :param description: The description for the new proposal (defaults to
>              the current description).
> +        :param commit_message: The commit message for the new proposal (defaults
> +            to the current commit message).

You also need to add the actual argument, and it's best to keep the docstring in the order the arguments are taken to avoid confusion.

>          """
>  
>      @operation_parameters(
> 
> === modified file 'lib/lp/code/model/branchmergeproposal.py'
> --- lib/lp/code/model/branchmergeproposal.py	2015-06-10 10:25:28 +0000
> +++ lib/lp/code/model/branchmergeproposal.py	2015-06-12 02:31:22 +0000
> @@ -634,8 +634,8 @@
>          self.date_merged = date_merged
>  
>      def resubmit(self, registrant, merge_source=None, merge_target=None,
> -                 merge_prerequisite=DEFAULT, description=None,
> -                 break_link=False):
> +                 merge_prerequisite=DEFAULT, commit_message=None,
> +                 description=None, break_link=False):
>          """See `IBranchMergeProposal`."""
>          if merge_source is None:
>              merge_source = self.merge_source
> @@ -649,6 +649,8 @@
>                  raise BranchMergeProposalExists(proposal)
>          if merge_prerequisite is DEFAULT:
>              merge_prerequisite = self.merge_prerequisite
> +        if commit_message is None:
> +            commit_message = self.commit_message
>          if description is None:
>              description = self.description
>          # You can transition from REJECTED to SUPERSEDED, but
> @@ -665,6 +667,7 @@
>              registrant=registrant,
>              merge_target=merge_target,
>              merge_prerequisite=merge_prerequisite,
> +            commit_message=commit_message,
>              description=description,
>              needs_review=True, review_requests=review_requests)
>          if not break_link:
> 
> === modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
> --- lib/lp/code/model/tests/test_branchmergeproposal.py	2015-05-26 07:39:49 +0000
> +++ lib/lp/code/model/tests/test_branchmergeproposal.py	2015-06-12 02:31:22 +0000
> @@ -1689,6 +1689,13 @@
>          revised = original.resubmit(original.registrant, description='foo')
>          self.assertEqual('foo', revised.description)
>  
> +    def test_resubmit_preserves_commit(self):
> +        """Resubmit preserves commit message."""
> +        original = self.factory.makeBranchMergeProposal()
> +        self.useContext(person_logged_in(original.registrant))
> +        revised = original.resubmit(original.registrant)
> +        self.assertEqual(original.commit_message, revised.commit_message)
> +
>      def test_resubmit_breaks_link(self):
>          """Resubmit breaks link, if specified."""
>          original = self.factory.makeBranchMergeProposal()
> 
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py	2015-06-11 08:06:28 +0000
> +++ lib/lp/testing/factory.py	2015-06-12 02:31:22 +0000
> @@ -1435,8 +1435,8 @@
>                                  set_state=None, prerequisite_branch=None,
>                                  product=None, initial_comment=None,
>                                  source_branch=None, date_created=None,
> -                                description=None, reviewer=None,
> -                                merged_revno=None):
> +                                commit_message=None, description=None,
> +                                reviewer=None, merged_revno=None):
>          """Create a proposal to merge based on anonymous branches."""
>          if target_branch is not None:
>              target_branch = removeSecurityProxy(target_branch)
> @@ -1452,9 +1452,11 @@
>              target_branch = self.makeProductBranch(product)
>              target = target_branch.target
>  
> -        # Fall back to initial_comment for description.
> +        # Fall back to initial_comment for description and commit_message.
>          if description is None:
>              description = initial_comment
> +        if commit_message is None:
> +            commit_message = initial_comment
>  
>          if target_branch is None:
>              target_branch = self.makeBranchTargetBranch(target)
> @@ -1468,7 +1470,7 @@
>          proposal = source_branch.addLandingTarget(
>              registrant, target_branch, review_requests=review_requests,
>              merge_prerequisite=prerequisite_branch, description=description,
> -            date_created=date_created)
> +            commit_message=commit_message, date_created=date_created)
>  
>          unsafe_proposal = removeSecurityProxy(proposal)
>          unsafe_proposal.merged_revno = merged_revno
> @@ -1497,8 +1499,8 @@
>                                        set_state=None, prerequisite_ref=None,
>                                        target=_DEFAULT, initial_comment=None,
>                                        source_ref=None, date_created=None,
> -                                      description=None, reviewer=None,
> -                                      merged_revision_id=None):
> +                                      commit_message=None, description=None,
> +                                      reviewer=None, merged_revision_id=None):
>          """Create a proposal to merge based on anonymous branches."""
>          if target is not _DEFAULT:
>              pass
> @@ -1514,9 +1516,11 @@
>              [target_ref] = self.makeGitRefs(target=target)
>              target = target_ref.target
>  
> -        # Fall back to initial_comment for description.
> +        # Fall back to initial_comment for description and commit_message.
>          if description is None:
>              description = initial_comment
> +        if commit_message is None:
> +            commit_message = initial_comment
>  
>          if target_ref is None:
>              [target_ref] = self.makeGitRefs(target=target)
> @@ -1530,7 +1534,7 @@
>          proposal = source_ref.addLandingTarget(
>              registrant, target_ref, review_requests=review_requests,
>              merge_prerequisite=prerequisite_ref, description=description,
> -            date_created=date_created)
> +            commit_message=commit_message, date_created=date_created)
>  
>          unsafe_proposal = removeSecurityProxy(proposal)
>          unsafe_proposal.merged_revision_id = merged_revision_id
> 


-- 
https://code.launchpad.net/~blr/launchpad/recycle-commit-message/+merge/261793
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.