Re: [PATCH] REST: allow fetching of subject prefixes (categories)

[Andrew Donnellan]

On 21/02/17 16:16, Daniel Axtens wrote:

We could possibly move this into the detail view. Andrew or Russell:
would that work for you?


That's fine by me.

--
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] REST: allow fetching of subject prefixes (categories)

[Daniel Axtens]

>> +def get_categories(self, instance):
>> +return clean_subject(instance.name)[1]
>
> This is going to run for 'page_size' patches on the list operation. I
> don't know how expensive each run is but seeing as there are regexes
> involved I doubt it's free. I wonder if we should access this via a
> 'cached_property' field on 'Submission'?

So I had a look at this: the cached property is calculated each time I
hit refresh, so I don't think we're going to get a big win here by
moving this logic.

We don't have a caching layer that would store these things across
requests, and we don't have any logic for invalidating them, so there's
no easy fix to this. (I'm pretty sure putting it in the database would
be even slower!)

We could possibly move this into the detail view. Andrew or Russell:
would that work for you?

Regards,
Daniel

>
>>  def get_mbox(self, instance):
>>  request = self.context.get('request')
>> @@ -100,10 +105,10 @@ class
>> PatchListSerializer(HyperlinkedModelSerializer):
>>  fields = ('id', 'url', 'project', 'msgid', 'date', 'name',
>>    'commit_ref', 'pull_url', 'state', 'archived',
>> 'hash',
>>    'submitter', 'delegate', 'mbox', 'series',
>> 'check', 'checks',
>> -  'tags')
>> +  'tags', 'categories')
>
> I'm not sure about 'categories' - this will also return things like the
> version ('v2') and the number of the patch if it's in a series ('1/5'),
> correct? Labels might be more valid, if so (though I had planned to use
> that name for a more significant feature in v2.1 or so [1]).
>
>>  read_only_fields = ('project', 'msgid', 'date', 'name',
>> 'hash',
>>  'submitter', 'mbox', 'mbox', 'series',
>> 'check',
>> -'checks', 'tags')
>> +'checks', 'tags', 'categories')
>
> Do we want to be able to filter on these? I don't know if you can do
> this for non-model fields though...
>
>>  extra_kwargs = {
>>  'url': {'view_name': 'api-patch-detail'},
>>  'project': {'view_name': 'api-project-detail'},
>
> Stephen
>
> [1] https://github.com/getpatchwork/patchwork/issues/22
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] REST: allow fetching of subject prefixes (categories)

[Daniel Axtens]

Hi all,

> On 14/02/17 21:59, Stephen Finucane wrote:
 I'm not sure about 'categories' - this will also return things like
 the
 version ('v2') and the number of the patch if it's in a series
 ('1/5'),
 correct? Labels might be more valid, if so (though I had planned to
 use
 that name for a more significant feature in v2.1 or so [1]).
>>>
>>> That's valid. I'm happy to leave 'labels' if you have designs on that
>>> for future. How about 'prefixes'? That also works well for leaving in
>>> version and series markers - they're all prefixes.
>>
>> Prefixes sounds good to me. Given that labels will probably function
>> quite similarly [*], it's likely that they'll completely replace this
>> in a future version of Patchwork and it would be best to avoid any
>> confusion over the two.
>
> ACK, prefixes sounds good to me. (The labels concept looks great, can't 
> wait!)


 Do we want to be able to filter on these? I don't know if you can
 do
 this for non-model fields though...
>>>
>>> At the moment, no. The use case is our CI system where we want to be
>>> able to take each patch one by one and figure out which tree to apply
>>> it
>>> to. So we don't need to filter on categories/prefixes.
>>
>> Yeah, I'm not too bothered about this unless it's easy to do.
>>
>> I do have one final question: this information is already available in
>> the 'name' field, right? What extra functionality would this give us
>> other than slight simplification of the client side code (though maybe
>> that's enough of a win by itself)?
>
> Making life easier for clients usually is a win in and of itself :) But 
> additionally it's more semantically meaningful to expose this as a 
> separate field.

Thanks all for your comments, I will do v2 as soon as I get some spare
cycles.

Regards,
Daniel

>
> -- 
> Andrew Donnellan  OzLabs, ADL Canberra
> andrew.donnel...@au1.ibm.com  IBM Australia Limited
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] REST: allow fetching of subject prefixes (categories)

[Andrew Donnellan]

On 14/02/17 21:59, Stephen Finucane wrote:

I'm not sure about 'categories' - this will also return things like
the
version ('v2') and the number of the patch if it's in a series
('1/5'),
correct? Labels might be more valid, if so (though I had planned to
use
that name for a more significant feature in v2.1 or so [1]).


That's valid. I'm happy to leave 'labels' if you have designs on that
for future. How about 'prefixes'? That also works well for leaving in
version and series markers - they're all prefixes.


Prefixes sounds good to me. Given that labels will probably function
quite similarly [*], it's likely that they'll completely replace this
in a future version of Patchwork and it would be best to avoid any
confusion over the two.


ACK, prefixes sounds good to me. (The labels concept looks great, can't 
wait!)





 read_only_fields = ('project', 'msgid', 'date', 'name',
'hash',
 'submitter', 'mbox', 'mbox',
'series',
'check',
-'checks', 'tags')
+'checks', 'tags', 'categories')


Do we want to be able to filter on these? I don't know if you can
do
this for non-model fields though...


At the moment, no. The use case is our CI system where we want to be
able to take each patch one by one and figure out which tree to apply
it
to. So we don't need to filter on categories/prefixes.


Yeah, I'm not too bothered about this unless it's easy to do.

I do have one final question: this information is already available in
the 'name' field, right? What extra functionality would this give us
other than slight simplification of the client side code (though maybe
that's enough of a win by itself)?


Making life easier for clients usually is a win in and of itself :) But 
additionally it's more semantically meaningful to expose this as a 
separate field.


--
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] REST: allow fetching of subject prefixes (categories)

[Stephen Finucane]

On Tue, 2017-02-14 at 08:47 +1100, Daniel Axtens wrote:
> Hi Stephen,
> 
> > On Sat, 2017-02-11 at 21:55 +1100, Daniel Axtens wrote:
> > > Some mailing lists accept patches for multiple projects, and use
> > > a subject prefix to differentiate the projects.
> > 
> > A couple of comments on this starting with this one: should we not
> > be
> > considering patches like this as belonging to a different 'Project'
> > in
> > Patchwork? If we don't do already support something, maybe we
> > should?
> > 
> > Could you provide an example of a mailing list that does this?
> 
> Yes - OpenBMC does this. The one mailing list supports the OpenBMC
> project, and within that they deal with code for several different
> components - the kernel, u-boot, etc:
> 
> https://patchwork.ozlabs.org/project/openbmc/list/?state=*
> 
> They (generally!) use these subject prefixes to distinguish between
> patches for different components/projects.

Lovely! :)

> > > +def get_categories(self, instance):
> > > +return clean_subject(instance.name)[1]
> > 
> > This is going to run for 'page_size' patches on the list operation.
> > I
> > don't know how expensive each run is but seeing as there are
> > regexes
> > involved I doubt it's free. I wonder if we should access this via a
> > 'cached_property' field on 'Submission'?
> 
> That's a good idea. I'll spin a v2 with that.
> 
> > 
> > >  def get_mbox(self, instance):
> > >  request = self.context.get('request')
> > > @@ -100,10 +105,10 @@ class
> > > PatchListSerializer(HyperlinkedModelSerializer):
> > >  fields = ('id', 'url', 'project', 'msgid', 'date',
> > > 'name',
> > >    'commit_ref', 'pull_url', 'state', 'archived',
> > > 'hash',
> > >    'submitter', 'delegate', 'mbox', 'series',
> > > 'check', 'checks',
> > > -  'tags')
> > > +  'tags', 'categories')
> > 
> > I'm not sure about 'categories' - this will also return things like
> > the
> > version ('v2') and the number of the patch if it's in a series
> > ('1/5'),
> > correct? Labels might be more valid, if so (though I had planned to
> > use
> > that name for a more significant feature in v2.1 or so [1]).
> 
> That's valid. I'm happy to leave 'labels' if you have designs on that
> for future. How about 'prefixes'? That also works well for leaving in
> version and series markers - they're all prefixes.

Prefixes sounds good to me. Given that labels will probably function
quite similarly [*], it's likely that they'll completely replace this
in a future version of Patchwork and it would be best to avoid any
confusion over the two.

> > >  read_only_fields = ('project', 'msgid', 'date', 'name',
> > > 'hash',
> > >  'submitter', 'mbox', 'mbox',
> > > 'series',
> > > 'check',
> > > -'checks', 'tags')
> > > +'checks', 'tags', 'categories')
> > 
> > Do we want to be able to filter on these? I don't know if you can
> > do
> > this for non-model fields though...
> 
> At the moment, no. The use case is our CI system where we want to be
> able to take each patch one by one and figure out which tree to apply
> it
> to. So we don't need to filter on categories/prefixes.

Yeah, I'm not too bothered about this unless it's easy to do.

I do have one final question: this information is already available in
the 'name' field, right? What extra functionality would this give us
other than slight simplification of the client side code (though maybe
that's enough of a win by itself)?

Cheers,
Stephen

[*] The last time we discussed labels on the mailing list, it was
suggested that we display labels inline in the subject by simply adding
them to the list of existing prefixes. As such, labels ~= custom
prefixes.
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] REST: allow fetching of subject prefixes (categories)

[Andrew Donnellan]

On 14/02/17 08:47, Daniel Axtens wrote:

Could you provide an example of a mailing list that does this?


Yes - OpenBMC does this. The one mailing list supports the OpenBMC
project, and within that they deal with code for several different
components - the kernel, u-boot, etc:


Kernel netdev also tends to use subject prefixes to specify when they're 
sending a patch for -next vs stable, as well as a handful of userspace 
applications that they maintain, like iproute2.


http://patchwork.ozlabs.org/project/netdev/list/


Do we want to be able to filter on these? I don't know if you can do
this for non-model fields though...


At the moment, no. The use case is our CI system where we want to be
able to take each patch one by one and figure out which tree to apply it
to. So we don't need to filter on categories/prefixes.


I think it would be useful to implement regardless.

--
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] REST: allow fetching of subject prefixes (categories)

[Russell Currey]

On Tue, 2017-02-14 at 08:47 +1100, Daniel Axtens wrote:
> Hi Stephen,
> 
> > On Sat, 2017-02-11 at 21:55 +1100, Daniel Axtens wrote:
> > > Some mailing lists accept patches for multiple projects, and use
> > > a subject prefix to differentiate the projects.
> > 
> > A couple of comments on this starting with this one: should we not be
> > considering patches like this as belonging to a different 'Project' in
> > Patchwork? If we don't do already support something, maybe we should?
> > 
> > Could you provide an example of a mailing list that does this?
> 
> Yes - OpenBMC does this. The one mailing list supports the OpenBMC
> project, and within that they deal with code for several different
> components - the kernel, u-boot, etc:
> 
> https://patchwork.ozlabs.org/project/openbmc/list/?state=*
> 
> They (generally!) use these subject prefixes to distinguish between
> patches for different components/projects.
> 
> > > +def get_categories(self, instance):
> > > +return clean_subject(instance.name)[1]
> > 
> > This is going to run for 'page_size' patches on the list operation. I
> > don't know how expensive each run is but seeing as there are regexes
> > involved I doubt it's free. I wonder if we should access this via a
> > 'cached_property' field on 'Submission'?
> 
> That's a good idea. I'll spin a v2 with that.
> 
> > 
> > >  def get_mbox(self, instance):
> > >  request = self.context.get('request')
> > > @@ -100,10 +105,10 @@ class
> > > PatchListSerializer(HyperlinkedModelSerializer):
> > >  fields = ('id', 'url', 'project', 'msgid', 'date', 'name',
> > >    'commit_ref', 'pull_url', 'state', 'archived',
> > > 'hash',
> > >    'submitter', 'delegate', 'mbox', 'series',
> > > 'check', 'checks',
> > > -  'tags')
> > > +  'tags', 'categories')
> > 
> > I'm not sure about 'categories' - this will also return things like the
> > version ('v2') and the number of the patch if it's in a series ('1/5'),
> > correct? Labels might be more valid, if so (though I had planned to use
> > that name for a more significant feature in v2.1 or so [1]).
> 
> That's valid. I'm happy to leave 'labels' if you have designs on that
> for future. How about 'prefixes'? That also works well for leaving in
> version and series markers - they're all prefixes.
> 
> > 
> > >  read_only_fields = ('project', 'msgid', 'date', 'name',
> > > 'hash',
> > >  'submitter', 'mbox', 'mbox', 'series',
> > > 'check',
> > > -'checks', 'tags')
> > > +'checks', 'tags', 'categories')
> > 
> > Do we want to be able to filter on these? I don't know if you can do
> > this for non-model fields though...
> 
> At the moment, no. The use case is our CI system where we want to be
> able to take each patch one by one and figure out which tree to apply it
> to. So we don't need to filter on categories/prefixes.

Even if *we* don't, it still seems like a good idea, i.e. for someone who is
only interested in patches for a specific category.  Having multiple projects on
the same list is dumb, but I've seen it enough times that it'd be good to
somewhat support.

> 
> Regards,
> Daniel
> 
> > Stephen
> > 
> > [1] https://github.com/getpatchwork/patchwork/issues/22

___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] REST: allow fetching of subject prefixes (categories)

[Daniel Axtens]

Hi Stephen,

> On Sat, 2017-02-11 at 21:55 +1100, Daniel Axtens wrote:
>> Some mailing lists accept patches for multiple projects, and use
>> a subject prefix to differentiate the projects.
>
> A couple of comments on this starting with this one: should we not be
> considering patches like this as belonging to a different 'Project' in
> Patchwork? If we don't do already support something, maybe we should?
>
> Could you provide an example of a mailing list that does this?

Yes - OpenBMC does this. The one mailing list supports the OpenBMC
project, and within that they deal with code for several different
components - the kernel, u-boot, etc:

https://patchwork.ozlabs.org/project/openbmc/list/?state=*

They (generally!) use these subject prefixes to distinguish between
patches for different components/projects.

>> +def get_categories(self, instance):
>> +return clean_subject(instance.name)[1]
>
> This is going to run for 'page_size' patches on the list operation. I
> don't know how expensive each run is but seeing as there are regexes
> involved I doubt it's free. I wonder if we should access this via a
> 'cached_property' field on 'Submission'?

That's a good idea. I'll spin a v2 with that.

>
>>  def get_mbox(self, instance):
>>  request = self.context.get('request')
>> @@ -100,10 +105,10 @@ class
>> PatchListSerializer(HyperlinkedModelSerializer):
>>  fields = ('id', 'url', 'project', 'msgid', 'date', 'name',
>>    'commit_ref', 'pull_url', 'state', 'archived',
>> 'hash',
>>    'submitter', 'delegate', 'mbox', 'series',
>> 'check', 'checks',
>> -  'tags')
>> +  'tags', 'categories')
>
> I'm not sure about 'categories' - this will also return things like the
> version ('v2') and the number of the patch if it's in a series ('1/5'),
> correct? Labels might be more valid, if so (though I had planned to use
> that name for a more significant feature in v2.1 or so [1]).

That's valid. I'm happy to leave 'labels' if you have designs on that
for future. How about 'prefixes'? That also works well for leaving in
version and series markers - they're all prefixes.

>
>>  read_only_fields = ('project', 'msgid', 'date', 'name',
>> 'hash',
>>  'submitter', 'mbox', 'mbox', 'series',
>> 'check',
>> -'checks', 'tags')
>> +'checks', 'tags', 'categories')
>
> Do we want to be able to filter on these? I don't know if you can do
> this for non-model fields though...

At the moment, no. The use case is our CI system where we want to be
able to take each patch one by one and figure out which tree to apply it
to. So we don't need to filter on categories/prefixes.

Regards,
Daniel

> Stephen
>
> [1] https://github.com/getpatchwork/patchwork/issues/22
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] REST: allow fetching of subject prefixes (categories)

[Stephen Finucane]

On Sat, 2017-02-11 at 21:55 +1100, Daniel Axtens wrote:
> Some mailing lists accept patches for multiple projects, and use
> a subject prefix to differentiate the projects.

A couple of comments on this starting with this one: should we not be
considering patches like this as belonging to a different 'Project' in
Patchwork? If we don't do already support something, maybe we should?

Could you provide an example of a mailing list that does this?

> Therefore, for snowpatch, it's useful to be able to fetch the
> subject prefixes. Snowpatch calls these 'categories', so lets
> adopt their terminology.
> 
> Export categeories in the REST API.
> 
> Signed-off-by: Daniel Axtens 
> ---
>  patchwork/api/patch.py | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index 1a7be584d0be..5536c41079b8 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -31,6 +31,7 @@ from patchwork.api.base import PatchworkPermission
>  from patchwork.api.filters import PatchFilter
>  from patchwork.models import Patch
>  from patchwork.models import State
> +from patchwork.parser import clean_subject
>  
>  
>  def format_state_name(state):
> @@ -76,6 +77,10 @@ class
> PatchListSerializer(HyperlinkedModelSerializer):
>  tags = SerializerMethodField()
>  check = SerializerMethodField()
>  checks = SerializerMethodField()
> +categories = SerializerMethodField()
> +
> +def get_categories(self, instance):
> +return clean_subject(instance.name)[1]

This is going to run for 'page_size' patches on the list operation. I
don't know how expensive each run is but seeing as there are regexes
involved I doubt it's free. I wonder if we should access this via a
'cached_property' field on 'Submission'?

>  def get_mbox(self, instance):
>  request = self.context.get('request')
> @@ -100,10 +105,10 @@ class
> PatchListSerializer(HyperlinkedModelSerializer):
>  fields = ('id', 'url', 'project', 'msgid', 'date', 'name',
>    'commit_ref', 'pull_url', 'state', 'archived',
> 'hash',
>    'submitter', 'delegate', 'mbox', 'series',
> 'check', 'checks',
> -  'tags')
> +  'tags', 'categories')

I'm not sure about 'categories' - this will also return things like the
version ('v2') and the number of the patch if it's in a series ('1/5'),
correct? Labels might be more valid, if so (though I had planned to use
that name for a more significant feature in v2.1 or so [1]).

>  read_only_fields = ('project', 'msgid', 'date', 'name',
> 'hash',
>  'submitter', 'mbox', 'mbox', 'series',
> 'check',
> -'checks', 'tags')
> +'checks', 'tags', 'categories')

Do we want to be able to filter on these? I don't know if you can do
this for non-model fields though...

>  extra_kwargs = {
>  'url': {'view_name': 'api-patch-detail'},
>  'project': {'view_name': 'api-project-detail'},

Stephen

[1] https://github.com/getpatchwork/patchwork/issues/22
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] REST: allow fetching of subject prefixes (categories)

[Russell Currey]

On Sat, 2017-02-11 at 21:55 +1100, Daniel Axtens wrote:
> Some mailing lists accept patches for multiple projects, and use
> a subject prefix to differentiate the projects.
> 
> Therefore, for snowpatch, it's useful to be able to fetch the
> subject prefixes. Snowpatch calls these 'categories', so lets
> adopt their terminology.
> 
> Export categeories in the REST API.
> 
> Signed-off-by: Daniel Axtens 
> ---

Yeah, this is very useful for mailing lists that receive patches for multiple
projects. I don't really care what terminology patchwork uses, though.

It might also be useful to have delegation rules based on these categories if
that's not already a thing (I don't know anything about delegation)

___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork