launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
> --- lib/lp/code/browser/tests/test_branchlisting.py	2015-12-11 04:40:07 +0000
> +++ lib/lp/code/browser/tests/test_branchlisting.py	2016-06-28 21:11:44 +0000
> @@ -389,7 +387,6 @@
>          page = self.get_branch_list_page()
>          self.assertThat(page, Not(snaps_matcher))
>          with feature_flags():

I think you can drop this context manager block, and also adjust the comment at the top of this method.

> -            set_feature_flag(SNAP_FEATURE_FLAG, u'on')
>              page = self.get_branch_list_page()
>              if IPerson.providedBy(self.default_target):
>                  self.assertThat(page, snaps_matcher)
> 
> === modified file 'lib/lp/snappy/browser/hassnaps.py'
> --- lib/lp/snappy/browser/hassnaps.py	2016-02-05 14:36:18 +0000
> +++ lib/lp/snappy/browser/hassnaps.py	2016-06-28 21:11:44 +0000
> @@ -42,12 +41,9 @@
>          # Only enabled if the general snap feature flag is enabled
>          # for public contexts and additionally if the snap_private
>          # flag is enabled for private contexts.

This comment is no longer quite accurate.

> -        if not bool(getFeatureFlag(SNAP_FEATURE_FLAG)):
> -            enabled = False
> -        else:
> -            enabled = (
> -                not self.context.private or
> -                bool(getFeatureFlag(SNAP_PRIVATE_FEATURE_FLAG)))
> +        enabled = (
> +            not self.context.private or
> +            bool(getFeatureFlag(SNAP_PRIVATE_FEATURE_FLAG)))
>  
>          text = 'Create snap package'
>          return Link('+new-snap', text, enabled=enabled, icon='add')
> @@ -66,9 +62,7 @@
>  
>      @property
>      def show_snap_information(self):
> -        return (
> -            bool(getFeatureFlag(SNAP_FEATURE_FLAG)) or
> -            not self.snaps.is_empty())
> +        return not self.snaps.is_empty()

This is only used in one place.  I'd be inclined to inline it now (i.e. tal:condition="not: view/snaps/is_empty").

>  
>      @property
>      def snaps_link(self):
> 
> === modified file 'lib/lp/snappy/browser/tests/test_snap.py'
> --- lib/lp/snappy/browser/tests/test_snap.py	2016-05-28 00:21:40 +0000
> +++ lib/lp/snappy/browser/tests/test_snap.py	2016-06-28 21:11:44 +0000
> @@ -137,7 +128,6 @@
>          branch = self.factory.makeAnyBranch(
>              owner=owner, information_type=InformationType.USERDATA)
>          with feature_flags():

Is this context manager still needed?

> -            set_feature_flag(SNAP_FEATURE_FLAG, u'on')
>              with person_logged_in(owner):
>                  self.assertRaises(
>                      SnapPrivateFeatureDisabled, create_initialized_view,
> 
> === modified file 'lib/lp/snappy/browser/tests/test_snaplisting.py'
> --- lib/lp/snappy/browser/tests/test_snaplisting.py	2016-05-18 12:15:05 +0000
> +++ lib/lp/snappy/browser/tests/test_snaplisting.py	2016-06-28 21:11:44 +0000
> @@ -39,8 +37,7 @@
>          We do things this way rather than by calling self.useFixture because
>          opening a URL in a test browser loses the thread-local feature flag.
>          """
> -        with FeatureFixture({SNAP_FEATURE_FLAG: u"on"}):
> -            return self.factory.makeSnap(**kwargs)
> +        return self.factory.makeSnap(**kwargs)

The method docstring is no longer accurate.  At this point this method is no longer pulling its weight anyway, so just change self.makeSnap to self.factory.makeSnap throughout this class and delete the method.

>  
>      def assertSnapsLink(self, context, link_text, link_has_context=False,
>                          **kwargs):
> 
> === modified file 'lib/lp/snappy/tests/test_snap.py'
> --- lib/lp/snappy/tests/test_snap.py	2016-05-17 12:47:34 +0000
> +++ lib/lp/snappy/tests/test_snap.py	2016-06-28 21:11:44 +0000
> @@ -86,18 +83,10 @@
>  
>      layer = LaunchpadZopelessLayer
>  
> -    def test_feature_flag_disabled(self):
> -        # Without a feature flag, we will not create new Snaps.
> -        person = self.factory.makePerson()
> -        self.assertRaises(
> -            SnapFeatureDisabled, getUtility(ISnapSet).new,
> -            person, person, None, None, branch=self.factory.makeAnyBranch())
> -
>      def test_private_feature_flag_disabled(self):
>          # Without a private feature flag, we will not create new private Snaps.
>          person = self.factory.makePerson()
>          with feature_flags():

Is this context manager still needed?

> -            set_feature_flag(SNAP_FEATURE_FLAG, u'on')
>              self.assertRaises(
>                  SnapPrivateFeatureDisabled, getUtility(ISnapSet).new,
>                  person, person, None, None,


-- 
https://code.launchpad.net/~maxiberta/launchpad/snappy-sampledata/+merge/298562
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.