[Patchew-devel] [PATCH] Refactoring of UpdateProjectHeadView.handle()

Shubham Jain posted 1 patch 5 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/patchew-ci tags/patchew/20180522191321.28691-1-shubhamjain7495@gmail.com
There is a newer version of this series
api/models.py | 42 ++++++++++++++++++++++++++++++++++++++++++
api/views.py  | 25 +------------------------
2 files changed, 43 insertions(+), 24 deletions(-)
[Patchew-devel] [PATCH] Refactoring of UpdateProjectHeadView.handle()
Posted by Shubham Jain 5 years, 11 months ago
moved and refactored the UpdateProjectHeadView.handle() into method of api.models.Project so that it can be re-used in rest conversion of update-project-head
---
 api/models.py | 42 ++++++++++++++++++++++++++++++++++++++++++
 api/views.py  | 25 +------------------------
 2 files changed, 43 insertions(+), 24 deletions(-)

diff --git a/api/models.py b/api/models.py
index d602cb7..be1e2be 100644
--- a/api/models.py
+++ b/api/models.py
@@ -44,6 +44,33 @@ def load_blob_json(name):
         logging.error('Failed to load blob %s: %s' %(name, e))
         return None
 
+def get_series_to_be_updated(project_name, message_ids):
+    updated_series = []
+    for msgid in message_ids:
+        if msgid.startswith("<") and msgid.endswith(">"):
+            msgid = msgid[1:-1]
+        p = Project.objects.get(name=project_name)
+        mo = Message.objects.filter(project=p, message_id=msgid,
+                                        is_merged=False).first()
+        if not mo:
+            continue
+        mo.is_merged = True
+        mo.save()
+        s = mo.get_series_head()
+        if s:
+            updated_series.append(s)
+    return updated_series
+
+def change_merge_status(series):
+    merged = True
+    for p in series.get_patches():
+        if not p.is_merged:
+            merged = False
+            break
+    if merged:
+        series.is_merged = True
+        series.save()
+
 class Project(models.Model):
     name = models.CharField(max_length=1024, db_index=True, unique=True,
                             help_text="""The name of the project""")
@@ -171,6 +198,21 @@ class Project(models.Model):
     def get_subprojects(self):
         return Project.objects.filter(parent_project=self)
 
+    def get_project_head(self):
+        return self.get_property("git.head")
+
+    project_head = property(get_project_head)
+
+    def series_update(self, message_ids):
+        series_to_be_updated = get_series_to_be_updated(self.name, message_ids)
+        ret = len(series_to_be_updated)
+        for s in series_to_be_updated:
+            change_merge_status(s)
+        return ret
+
+    def set_project_head(self, new_head):
+        self.set_property("git.head", new_head)
+
 class ProjectProperty(models.Model):
     project = models.ForeignKey('Project', on_delete=models.CASCADE)
     name = models.CharField(max_length=1024, db_index=True)
diff --git a/api/views.py b/api/views.py
index c27cd5a..4cb10c1 100644
--- a/api/views.py
+++ b/api/views.py
@@ -123,30 +123,7 @@ class UpdateProjectHeadView(APILoginRequiredView):
         old_head_0 = po.get_property("git.head")
         if old_head_0 and old_head_0 != old_head:
             raise Exception("wrong old head")
-        ret = 0
-        updated_series = []
-        for msgid in message_ids:
-            if msgid.startswith("<") and msgid.endswith(">"):
-                msgid = msgid[1:-1]
-            mo = Message.objects.filter(project=po, message_id=msgid,
-                                        is_merged=False).first()
-            if not mo:
-                continue
-            ret += 1
-            mo.is_merged = True
-            mo.save()
-            s = mo.get_series_head()
-            if s:
-                updated_series.append(s)
-        for s in updated_series:
-            merged = True
-            for p in s.get_patches():
-                if not p.is_merged:
-                    merged = False
-                    break
-            if merged:
-                s.is_merged = True
-                s.save()
+        ret = po.series_update(message_ids)
         po.set_property("git.head", new_head)
         return ret
 
-- 
2.14.3 (Apple Git-98)

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH] Refactoring of UpdateProjectHeadView.handle()
Posted by Fam Zheng 5 years, 11 months ago
On Wed, 05/23 00:43, Shubham Jain wrote:
> moved and refactored the UpdateProjectHeadView.handle() into method of api.models.Project so that it can be re-used in rest conversion of update-project-head
> ---
>  api/models.py | 42 ++++++++++++++++++++++++++++++++++++++++++
>  api/views.py  | 25 +------------------------
>  2 files changed, 43 insertions(+), 24 deletions(-)
> 
> diff --git a/api/models.py b/api/models.py
> index d602cb7..be1e2be 100644
> --- a/api/models.py
> +++ b/api/models.py
> @@ -44,6 +44,33 @@ def load_blob_json(name):
>          logging.error('Failed to load blob %s: %s' %(name, e))
>          return None
>  
> +def get_series_to_be_updated(project_name, message_ids):

You can pass the Project object then ...

> +    updated_series = []
> +    for msgid in message_ids:
> +        if msgid.startswith("<") and msgid.endswith(">"):
> +            msgid = msgid[1:-1]
> +        p = Project.objects.get(name=project_name)

... this is not necessary.

> +        mo = Message.objects.filter(project=p, message_id=msgid,
> +                                        is_merged=False).first()
> +        if not mo:
> +            continue
> +        mo.is_merged = True
> +        mo.save()
> +        s = mo.get_series_head()
> +        if s:
> +            updated_series.append(s)
> +    return updated_series
> +
> +def change_merge_status(series):
> +    merged = True
> +    for p in series.get_patches():
> +        if not p.is_merged:
> +            merged = False
> +            break
> +    if merged:
> +        series.is_merged = True
> +        series.save()
> +
>  class Project(models.Model):
>      name = models.CharField(max_length=1024, db_index=True, unique=True,
>                              help_text="""The name of the project""")
> @@ -171,6 +198,21 @@ class Project(models.Model):
>      def get_subprojects(self):
>          return Project.objects.filter(parent_project=self)
>  
> +    def get_project_head(self):
> +        return self.get_property("git.head")
> +
> +    project_head = property(get_project_head)
> +
> +    def series_update(self, message_ids):
> +        series_to_be_updated = get_series_to_be_updated(self.name, message_ids)
> +        ret = len(series_to_be_updated)
> +        for s in series_to_be_updated:
> +            change_merge_status(s)
> +        return ret
> +
> +    def set_project_head(self, new_head):
> +        self.set_property("git.head", new_head)
> +
>  class ProjectProperty(models.Model):
>      project = models.ForeignKey('Project', on_delete=models.CASCADE)
>      name = models.CharField(max_length=1024, db_index=True)
> diff --git a/api/views.py b/api/views.py
> index c27cd5a..4cb10c1 100644
> --- a/api/views.py
> +++ b/api/views.py
> @@ -123,30 +123,7 @@ class UpdateProjectHeadView(APILoginRequiredView):
>          old_head_0 = po.get_property("git.head")
>          if old_head_0 and old_head_0 != old_head:
>              raise Exception("wrong old head")
> -        ret = 0
> -        updated_series = []
> -        for msgid in message_ids:
> -            if msgid.startswith("<") and msgid.endswith(">"):
> -                msgid = msgid[1:-1]
> -            mo = Message.objects.filter(project=po, message_id=msgid,
> -                                        is_merged=False).first()
> -            if not mo:
> -                continue
> -            ret += 1
> -            mo.is_merged = True
> -            mo.save()
> -            s = mo.get_series_head()
> -            if s:
> -                updated_series.append(s)
> -        for s in updated_series:
> -            merged = True
> -            for p in s.get_patches():
> -                if not p.is_merged:
> -                    merged = False
> -                    break
> -            if merged:
> -                s.is_merged = True
> -                s.save()
> +        ret = po.series_update(message_ids)
>          po.set_property("git.head", new_head)

Change this to po.set_project_head()?

>          return ret
>  
> -- 
> 2.14.3 (Apple Git-98)
> 
> _______________________________________________
> Patchew-devel mailing list
> Patchew-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/patchew-devel

Fam

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH] Refactoring of UpdateProjectHeadView.handle()
Posted by Paolo Bonzini 5 years, 11 months ago
On 22/05/2018 21:13, Shubham Jain wrote:
> moved and refactored the UpdateProjectHeadView.handle() into method of api.models.Project so that it can be re-used in rest conversion of update-project-head
> ---
>  api/models.py | 42 ++++++++++++++++++++++++++++++++++++++++++
>  api/views.py  | 25 +------------------------
>  2 files changed, 43 insertions(+), 24 deletions(-)
> 
> diff --git a/api/models.py b/api/models.py
> index d602cb7..be1e2be 100644
> --- a/api/models.py
> +++ b/api/models.py
> @@ -44,6 +44,33 @@ def load_blob_json(name):
>          logging.error('Failed to load blob %s: %s' %(name, e))
>          return None
>  
> +def get_series_to_be_updated(project_name, message_ids):
> +    updated_series = []
> +    for msgid in message_ids:
> +        if msgid.startswith("<") and msgid.endswith(">"):
> +            msgid = msgid[1:-1]
> +        p = Project.objects.get(name=project_name)
> +        mo = Message.objects.filter(project=p, message_id=msgid,
> +                                        is_merged=False).first()
> +        if not mo:
> +            continue
> +        mo.is_merged = True
> +        mo.save()
> +        s = mo.get_series_head()
> +        if s:
> +            updated_series.append(s)
> +    return updated_series
>
> +def change_merge_status(series):
> +    merged = True
> +    for p in series.get_patches():
> +        if not p.is_merged:
> +            merged = False
> +            break
> +    if merged:
> +        series.is_merged = True
> +        series.save()

Both methods are small, so I think it's okay to place the two algorithms
in series_update directly.  The advantage is that the global namespace
of api.models remains clean.

As an additional cleanup, the for loop in change_merge_status could be
written

    for p in series.get_patches():
        if not p.is_merged:
            break
    else:
        series.is_merged = True
        series.save()

>  class Project(models.Model):
>      name = models.CharField(max_length=1024, db_index=True, unique=True,
>                              help_text="""The name of the project""")
> @@ -171,6 +198,21 @@ class Project(models.Model):
>      def get_subprojects(self):
>          return Project.objects.filter(parent_project=self)
>  
> +    def get_project_head(self):
> +        return self.get_property("git.head")
> +
> +    project_head = property(get_project_head)
> +
> +    def series_update(self, message_ids):
> +        series_to_be_updated = get_series_to_be_updated(self.name, message_ids)
> +        ret = len(series_to_be_updated)
> +        for s in series_to_be_updated:
> +            change_merge_status(s)
> +        return ret
> +
> +    def set_project_head(self, new_head):
> +        self.set_property("git.head", new_head)

You're not using this method and the corresponding getter, and I think
it's okay to leave it in UpdateProjectHeadView.  But if you kept it, you
should use "project_head = property(get_project_head, set_project_head)".

Thanks,

Paolo

>  class ProjectProperty(models.Model):
>      project = models.ForeignKey('Project', on_delete=models.CASCADE)
>      name = models.CharField(max_length=1024, db_index=True)
> diff --git a/api/views.py b/api/views.py
> index c27cd5a..4cb10c1 100644
> --- a/api/views.py
> +++ b/api/views.py
> @@ -123,30 +123,7 @@ class UpdateProjectHeadView(APILoginRequiredView):
>          old_head_0 = po.get_property("git.head")
>          if old_head_0 and old_head_0 != old_head:
>              raise Exception("wrong old head")
> -        ret = 0
> -        updated_series = []
> -        for msgid in message_ids:
> -            if msgid.startswith("<") and msgid.endswith(">"):
> -                msgid = msgid[1:-1]
> -            mo = Message.objects.filter(project=po, message_id=msgid,
> -                                        is_merged=False).first()
> -            if not mo:
> -                continue
> -            ret += 1
> -            mo.is_merged = True
> -            mo.save()
> -            s = mo.get_series_head()
> -            if s:
> -                updated_series.append(s)
> -        for s in updated_series:
> -            merged = True
> -            for p in s.get_patches():
> -                if not p.is_merged:
> -                    merged = False
> -                    break
> -            if merged:
> -                s.is_merged = True
> -                s.save()
> +        ret = po.series_update(message_ids)
>          po.set_property("git.head", new_head)
>          return ret
>  
> 

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH] Refactoring of UpdateProjectHeadView.handle()
Posted by Shubham Jain 5 years, 11 months ago
> As an additional cleanup, the for loop in change_merge_status could be
> written
>
>     for p in series.get_patches():
>         if not p.is_merged:
>             break
>     else:
>         series.is_merged = True
>         series.save()

Wow, I didn't know else clause could be used with a loop as well. Thanks!
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel