[PATCH 3 of 5] [Simplification] on sort

Denis Laxalde denis.laxalde at logilab.fr
Tue Jun 11 09:25:58 CEST 2019


Julien Tayon a écrit :
> # HG changeset patch
> # User julien tayon <julien.tayon at logilab.fr>
> # Date 1559134954 -7200
> #      Wed May 29 15:02:34 2019 +0200
> # Node ID 41a7b43c31da05fea7a8a6e74d062a6352202a85
> # Parent  0d1516ed43f806125ef957d8a302b95702f3c051
> [Simplification] on sort
> 
> Since key is never used and I do have the strange feeling the order
> was meant to be on the type.
> 
> Explanation itemgetter(1) referes to rschema, thus to an object which
> does not support __lt__, __gt__ (ordering) has far as I can see, hence since
> it works it means it probably uses __hash__ (used to make object subscriptable),
> as a result order is random.
> 
> On the other hand type attributes is string, thus ... sortable
> 
> Tried to get in this part of the code using ?vid=owl using a pdb.set_trace
> to check I was unable to achieve this point.
> 
> diff --git a/cubicweb/web/schemaviewer.py b/cubicweb/web/schemaviewer.py
> --- a/cubicweb/web/schemaviewer.py
> +++ b/cubicweb/web/schemaviewer.py
> @@ -88,13 +88,9 @@ class SchemaViewer(object):
>              def skipped_relation_schema(rschema):
>                  return not (rschema.final or rschema.type in skiptypes)
>  
> -            relations = list(
> -                filter(
> -                    skipped_relation_schema,
> -                    sorted(schema.relations(), key=attrgetter('type'))
> -                ))
> -            keys = [(rschema.type, rschema) for rschema in relations]
> -            for key, rschema in sorted(keys, key=itemgetter(1)):
> +            for rschema in sorted(
> +                    filter(skipped_relation_schema, schema.relations()),
> +                    key=itemgetter('type')):

I'm confused by this patch because it applies on top of another change
of yours in the same series. Anyways...

If I try to understand your point, I also think we can totally drop
the "keys" variable but I however think that the "key=" option in
sorted() is a behavior change. Since I'm not sure what this code does,
and we have apparently no test for this, I'd rather not change this.

So I'd simply suggest the following change:

  diff --git a/cubicweb/web/schemaviewer.py b/cubicweb/web/schemaviewer.py
  --- a/cubicweb/web/schemaviewer.py
  +++ b/cubicweb/web/schemaviewer.py
  @@ -24,7 +24,7 @@ from logilab.common.ureports import Sect
   
   from yams.schema2dot import CARD_MAP
   from yams.schema import RelationDefinitionSchema
  -from operator import attrgetter, itemgetter
  +from operator import attrgetter
   
   TYPE_GETTER = attrgetter('type')
   
  @@ -88,8 +88,7 @@ class SchemaViewer(object):
               layout.append(rsection)
               relations = [rschema for rschema in sorted(schema.relations(), key=TYPE_GETTER)
                            if not (rschema.final or rschema.type in skiptypes)]
  -            keys = [(rschema.type, rschema) for rschema in relations]
  -            for key, rschema in sorted(keys, key=itemgetter(1)):
  +            for rschema in relations:
                   relstr = self.visit_relationschema(rschema)
                   rsection.append(relstr)
           return layout

What do you think?



More information about the cubicweb-devel mailing list