launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

I added just one suggestion. But apart from that, it's good.

Diff comments:

> diff --git a/lib/lp/code/interfaces/branch.py b/lib/lp/code/interfaces/branch.py
> index f0e9393..ccb4084 100644
> --- a/lib/lp/code/interfaces/branch.py
> +++ b/lib/lp/code/interfaces/branch.py
> @@ -1427,10 +1427,36 @@ class IBranchSet(Interface):
>          Return None if no match was found.
>          """
>  
> -    @collection_default_content()
> -    def getBranches(limit=50, eager_load=True):
> +    @call_with(user=REQUEST_USER)
> +    @operation_parameters(
> +        order_by_modified_date=Bool(

Wouldn't it be better to have this as `order_by=TextLine()` (for the lack of Enum), and `modified_date` being the only possible option for now? This way we can add other types of sorting in the future without adding more `order_by_{attribute_name}` boolean parameters.

> +            title=_("Order by modified date"),
> +            # order_by_modified_date=False currently returns branches in the
> +            # same order as True (descending date_last_modified followed by
> +            # descending ID), but we don't want to commit to this.
> +            description=_(
> +                "Return most-recently-modified branches first.  If False, "
> +                "the order of results is unspecified."),
> +            required=False),
> +        modified_since_date=Datetime(
> +            title=_("Modified since date"),
> +            description=_(
> +                "Return only branches whose `date_last_modified` is "
> +                "greater than or equal to this date.")))
> +    @operation_returns_collection_of(IBranch)
> +    @export_read_operation()
> +    @operation_for_version("devel")
> +    @collection_default_content(user=None, limit=50)
> +    def getBranches(user, order_by_modified_date=False,
> +                    modified_since_date=None, limit=None, eager_load=True):
>          """Return a collection of branches.
>  
> +        :param user: An `IPerson`.  Only branches visible by this user will
> +            be returned.
> +        :param order_by_modified_date: If True, order results by descending
> +            `date_last_modified`; if False, the order is unspecified.
> +        :param modified_since_date: If not None, return only branches whose
> +            `date_last_modified` is greater than this date.
>          :param eager_load: If True (the default because this is used in the
>              web service and it needs the related objects to create links)
>              eager load related objects (products, code imports etc).
> diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
> index eeac433..124ebaa 100644
> --- a/lib/lp/code/interfaces/gitrepository.py
> +++ b/lib/lp/code/interfaces/gitrepository.py
> @@ -989,16 +989,35 @@ class IGitRepositorySet(Interface):
>      @call_with(user=REQUEST_USER)
>      @operation_parameters(
>          target=Reference(
> -            title=_("Target"), required=True, schema=IHasGitRepositories))
> +            title=_("Target"), required=False, schema=IHasGitRepositories),
> +        order_by_modified_date=Bool(

If the previous comment about a generic `order_by` is accepted for bzr branches, this should follow the same pattern.

> +            title=_("Order by modified date"),
> +            # order_by_modified_date=False currently returns repositories
> +            # ordered by ID, but we don't want to commit to this.
> +            description=_(
> +                "Return most-recently-modified repositories first.  If False, "
> +                "the order of results is unspecified."),
> +            required=False),
> +        modified_since_date=Datetime(
> +            title=_("Modified since date"),
> +            description=_(
> +                "Return only repositories whose `date_last_modified` is "
> +                "greater than or equal to this date.")))
>      @operation_returns_collection_of(IGitRepository)
>      @export_read_operation()
>      @operation_for_version("devel")
> -    def getRepositories(user, target):
> +    def getRepositories(user, target=None, order_by_modified_date=False,
> +                        modified_since_date=None):
>          """Get all repositories for a target.
>  
>          :param user: An `IPerson`.  Only repositories visible by this user
>              will be returned.
> -        :param target: An `IHasGitRepositories`.
> +        :param target: An `IHasGitRepositories`, or None to get repositories
> +            for all targets.
> +        :param order_by_modified_date: If True, order results by descending
> +            `date_last_modified`; if False, the order is unspecified.
> +        :param modified_since_date: If not None, return only repositories
> +            whose `date_last_modified` is greater than this date.
>  
>          :return: A collection of `IGitRepository` objects.
>          """


-- 
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/380391
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:date-ordered-repositories into launchpad:master.