| Message ID | 1446914966-24803-1-git-send-email-damien.lespiau@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
> In: > > commit 544b8bbcc7ec80d94c96f181886c51b177530a95 > Author: Stephen Finucane <stephen.finucane@intel.com> > Date: Fri Aug 21 15:32:17 2015 +0100 > > trivial: Resolve PEP8 issues with 'management' > > I noted that it wasn't all trivial changes. And indeed using the > stdin/stdout wrappers change the intended behaviour by adding a new > line. > > The stderr wrapper also colors the line in red. Changed the last message > to be printed on stdout, seems more logical. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > patchwork/management/commands/retag.py | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/patchwork/management/commands/retag.py > b/patchwork/management/commands/retag.py > index f92648b..e67d099 100644 > --- a/patchwork/management/commands/retag.py > +++ b/patchwork/management/commands/retag.py > @@ -17,6 +17,7 @@ > # along with Patchwork; if not, write to the Free Software > # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 > USA > > +import sys > from django.core.management.base import BaseCommand > from patchwork.models import Patch > > @@ -38,6 +39,6 @@ class Command(BaseCommand): > for i, patch in enumerate(query.iterator()): > patch.refresh_tag_counts() > if (i % 10) == 0 or i == count: > - self.stdout.write('%06d/%06d\r' % (i, count)) > - self.stdout.flush() > - self.stderr.write('\ndone\n') > + sys.stdout.write('%06d/%06d\r' % (i, count)) > + sys.stdout.flush() > + sys.stdout.write('\ndone\n') Agree on stderr - > stdout, however Django documentation advises we use the BaseCommand versions hence my original change: https://docs.djangoproject.com/en/1.8/howto/custom-management-commands/#management-commands-output Can you just drop the trailing newline instead? > -- > 2.4.3 > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
On Mon, Nov 09, 2015 at 02:29:57AM +0000, Finucane, Stephen wrote: > > @@ -38,6 +39,6 @@ class Command(BaseCommand): > > for i, patch in enumerate(query.iterator()): > > patch.refresh_tag_counts() > > if (i % 10) == 0 or i == count: > > - self.stdout.write('%06d/%06d\r' % (i, count)) > > - self.stdout.flush() > > - self.stderr.write('\ndone\n') > > + sys.stdout.write('%06d/%06d\r' % (i, count)) > > + sys.stdout.flush() > > + sys.stdout.write('\ndone\n') > > Agree on stderr - > stdout, however Django documentation advises we > use the BaseCommand versions hence my original change: > > https://docs.djangoproject.com/en/1.8/howto/custom-management-commands/#management-commands-output > > Can you just drop the trailing newline instead? You're missing the point: > > I noted that it wasn't all trivial changes. And indeed using the > > stdin/stdout wrappers change the intended behaviour by adding a new > > line. self.stdout.write('%06d/%06d\r' % (i, count)) That will append a new line, where the intention was to have a progress indicator on one line ('\r').
> -----Original Message----- > From: Lespiau, Damien > Sent: Monday, November 9, 2015 4:54 AM > To: Finucane, Stephen > Cc: patchwork@lists.ozlabs.org > Subject: Re: [PATCH 1/2] retag: Don't use BaseCommand's stdin/stdout > wrappers > > On Mon, Nov 09, 2015 at 02:29:57AM +0000, Finucane, Stephen wrote: > > > @@ -38,6 +39,6 @@ class Command(BaseCommand): > > > for i, patch in enumerate(query.iterator()): > > > patch.refresh_tag_counts() > > > if (i % 10) == 0 or i == count: > > > - self.stdout.write('%06d/%06d\r' % (i, count)) > > > - self.stdout.flush() > > > - self.stderr.write('\ndone\n') > > > + sys.stdout.write('%06d/%06d\r' % (i, count)) > > > + sys.stdout.flush() > > > + sys.stdout.write('\ndone\n') > > > > Agree on stderr - > stdout, however Django documentation advises we > > use the BaseCommand versions hence my original change: > > > > https://docs.djangoproject.com/en/1.8/howto/custom-management- > commands/#management-commands-output > > > > Can you just drop the trailing newline instead? > > You're missing the point: > > > > I noted that it wasn't all trivial changes. And indeed using the > > > stdin/stdout wrappers change the intended behaviour by adding a new > > > line. > > self.stdout.write('%06d/%06d\r' % (i, count)) > > That will append a new line, where the intention was to have a progress > indicator on one line ('\r'). Ah, I get the point - I just misinterpreted the line ending character (it's a '\r', but I instinctively saw a '\n'). How about this? self.stdout.write('%06d/%06d\r' % (i, count), ending='') or this, if you prefer: self.stdout.write('%06d/%06d' % (i, count), ending='\r') I'm raising this issue just because the Django docs explicitly say to use these wrappers. Stephen > -- > Damien
On Mon, Nov 09, 2015 at 02:53:36PM +0000, Finucane, Stephen wrote: > Ah, I get the point - I just misinterpreted the line ending character > (it's a '\r', but I instinctively saw a '\n'). How about this? > > self.stdout.write('%06d/%06d\r' % (i, count), ending='') > > or this, if you prefer: > > self.stdout.write('%06d/%06d' % (i, count), ending='\r') > > I'm raising this issue just because the Django docs explicitly say to use Feel free to rewrite the patch this way if you wish, I don't mind either way.
> On Mon, Nov 09, 2015 at 02:53:36PM +0000, Finucane, Stephen wrote: > > Ah, I get the point - I just misinterpreted the line ending character > > (it's a '\r', but I instinctively saw a '\n'). How about this? > > > > self.stdout.write('%06d/%06d\r' % (i, count), ending='') > > > > or this, if you prefer: > > > > self.stdout.write('%06d/%06d' % (i, count), ending='\r') > > > > I'm raising this issue just because the Django docs explicitly say to use > > Feel free to rewrite the patch this way if you wish, I don't mind either > way. OK, sweet. I'm thinking that asking you this stuff is better than just doing it because I can make mistakes (I'd likely not have included the `ending` parameter, for example). Correct me if I'm wrong. I'll get this merged tonight. Stephen PS: Since we're talking, if you have time to address the remaining comments on the 'series' series it would be appreciated. I don't want to just do the rework without you input in case I miss something. It's one of three blockers for a 2.0 release, along with the Django 1.6 migrations (:() and interface integration of series (Web UI and XML-RPC, until we get REST sorted). I'm hoping 2.0 should be widely deployed, so it would be a nice kick to get this feature in.
On Mon, Nov 09, 2015 at 03:33:18PM +0000, Finucane, Stephen wrote: > PS: Since we're talking, if you have time to address the remaining comments > on the 'series' series it would be appreciated. I don't want to just do the > rework without you input in case I miss something. It's one of three blockers > for a 2.0 release, along with the Django 1.6 migrations (:() and interface > integration of series (Web UI and XML-RPC, until we get REST sorted). I'm > hoping 2.0 should be widely deployed, so it would be a nice kick to get this > feature in. Sorry, I don't have time to look at it.
> On Mon, Nov 09, 2015 at 03:33:18PM +0000, Finucane, Stephen wrote: > > PS: Since we're talking, if you have time to address the remaining > comments > > on the 'series' series it would be appreciated. I don't want to just do > the > > rework without you input in case I miss something. It's one of three > blockers > > for a 2.0 release, along with the Django 1.6 migrations (:() and > interface > > integration of series (Web UI and XML-RPC, until we get REST sorted). I'm > > hoping 2.0 should be widely deployed, so it would be a nice kick to get > this > > feature in. > > Sorry, I don't have time to look at it. OK. Are you happy for me to rework as appropriate, including notes and sign-offs in commit messages? Stephen
On Mon, Nov 09, 2015 at 03:39:09PM +0000, Finucane, Stephen wrote: > > On Mon, Nov 09, 2015 at 03:33:18PM +0000, Finucane, Stephen wrote: > > > PS: Since we're talking, if you have time to address the remaining > > comments > > > on the 'series' series it would be appreciated. I don't want to just do > > the > > > rework without you input in case I miss something. It's one of three > > blockers > > > for a 2.0 release, along with the Django 1.6 migrations (:() and > > interface > > > integration of series (Web UI and XML-RPC, until we get REST sorted). I'm > > > hoping 2.0 should be widely deployed, so it would be a nice kick to get > > this > > > feature in. > > > > Sorry, I don't have time to look at it. > > OK. Are you happy for me to rework as appropriate, including notes and > sign-offs in commit messages? I'd rather you don't change my commits but work on top of them, changing details in the db is also going to break compatibility, it'd be best not to do it.
> On Mon, Nov 09, 2015 at 03:39:09PM +0000, Finucane, Stephen wrote: > > > On Mon, Nov 09, 2015 at 03:33:18PM +0000, Finucane, Stephen wrote: > > > > PS: Since we're talking, if you have time to address the remaining > > > comments > > > > on the 'series' series it would be appreciated. I don't want to just > do > > > the > > > > rework without you input in case I miss something. It's one of three > > > blockers > > > > for a 2.0 release, along with the Django 1.6 migrations (:() and > > > interface > > > > integration of series (Web UI and XML-RPC, until we get REST sorted). > I'm > > > > hoping 2.0 should be widely deployed, so it would be a nice kick to > get > > > this > > > > feature in. > > > > > > Sorry, I don't have time to look at it. > > > > OK. Are you happy for me to rework as appropriate, including notes and > > sign-offs in commit messages? > > I'd rather you don't change my commits but work on top of them, changing > details in the db is also going to break compatibility, it'd be best not > to do it. I agree and the feature is too important to drop, but I do have concerns. The only way to address such concerns and still get the feature in is to discuss them with the contributor or go and fix them myself. They will have to be fixed in one commit to avoid the additional migration, which is more difficult for us due to the Django 1.6 support requirement :( If my points are valid, compatibility should be addressable on your end via a migration, which will be easier for your fork as you don't need to worry about Django 1.6? If you can squeeze in time to discuss then please do so. If not, I'll have to assume my concerns are valid, make the changes myself and break compatibility to ensure the feature makes it in. The latter option is rather undesirable, IMO. Stephen
On Mon, Nov 09, 2015 at 03:39:09PM +0000, Finucane, Stephen wrote: > > On Mon, Nov 09, 2015 at 03:33:18PM +0000, Finucane, Stephen wrote: > > > PS: Since we're talking, if you have time to address the remaining > > comments > > > on the 'series' series it would be appreciated. I don't want to just do > > the > > > rework without you input in case I miss something. It's one of three > > blockers > > > for a 2.0 release, along with the Django 1.6 migrations (:() and > > interface > > > integration of series (Web UI and XML-RPC, until we get REST sorted). I'm > > > hoping 2.0 should be widely deployed, so it would be a nice kick to get > > this > > > feature in. > > > > Sorry, I don't have time to look at it. > > OK. Are you happy for me to rework as appropriate, including notes and > sign-offs in commit messages? I forgot the most important part: I would strongly advice against merging the series support. This will make patchwork drop patches in certain corner cases. I'm still addressing those corner cases. The latest example I just fixed: http://lists.freedesktop.org/archives/wayland-devel/2015-November/025381.html Even without dropping patches -- there are a few known issues still -- the series/revision creation can sometimes be a bit unsettling because we are trying to guess the intent of the user. Last thing, series support hasn't reached the threshold I would consider good enough to make it into a release. At the very least the above has to be fixed, series status needs to happen (what is the state of a series based on the state of the underlying patches) and series filtering as well (just like patches, search capabilities and not display series with a state with action_required = True) HTH,
On 09 Nov 15:20, Damien Lespiau wrote: > On Mon, Nov 09, 2015 at 02:53:36PM +0000, Finucane, Stephen wrote: > > Ah, I get the point - I just misinterpreted the line ending character > > (it's a '\r', but I instinctively saw a '\n'). How about this? > > > > self.stdout.write('%06d/%06d\r' % (i, count), ending='') > > > > or this, if you prefer: > > > > self.stdout.write('%06d/%06d' % (i, count), ending='\r') > > > > I'm raising this issue just because the Django docs explicitly say to use > > Feel free to rewrite the patch this way if you wish, I don't mind either > way. Merged.
diff --git a/patchwork/management/commands/retag.py b/patchwork/management/commands/retag.py index f92648b..e67d099 100644 --- a/patchwork/management/commands/retag.py +++ b/patchwork/management/commands/retag.py @@ -17,6 +17,7 @@ # along with Patchwork; if not, write to the Free Software # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +import sys from django.core.management.base import BaseCommand from patchwork.models import Patch @@ -38,6 +39,6 @@ class Command(BaseCommand): for i, patch in enumerate(query.iterator()): patch.refresh_tag_counts() if (i % 10) == 0 or i == count: - self.stdout.write('%06d/%06d\r' % (i, count)) - self.stdout.flush() - self.stderr.write('\ndone\n') + sys.stdout.write('%06d/%06d\r' % (i, count)) + sys.stdout.flush() + sys.stdout.write('\ndone\n')
In: commit 544b8bbcc7ec80d94c96f181886c51b177530a95 Author: Stephen Finucane <stephen.finucane@intel.com> Date: Fri Aug 21 15:32:17 2015 +0100 trivial: Resolve PEP8 issues with 'management' I noted that it wasn't all trivial changes. And indeed using the stdin/stdout wrappers change the intended behaviour by adding a new line. The stderr wrapper also colors the line in red. Changed the last message to be printed on stdout, seems more logical. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- patchwork/management/commands/retag.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)