| Message ID | 1297185755.13357.106.camel@feioso |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Feb 08, 2011 at 03:22:35PM -0200, Guilherme Salgado wrote: > On Tue, 2011-02-08 at 12:39 -0200, Guilherme Salgado wrote: > > Hi, > > > > I was looking at how patchwork deals with pull requests and noticed that > > when it parses a message containing both a pull request and a patch, it > > will create two Patch() instances (one with the patch as its content and > > the other without content but with a pull_url): > > > > if patchbuf: > > mail_headers(mail) > > name = clean_subject(mail.get('Subject'), [project.linkname]) > > patch = Patch(name = name, content = patchbuf, > > date = mail_date(mail), headers = mail_headers(mail)) > > > > if pullurl: > > name = clean_subject(mail.get('Subject'), [project.linkname]) > > patch = Patch(name = name, pull_url = pullurl, > > date = mail_date(mail), headers = mail_headers(mail)) > > > > However, these are not automatically inserted in the DB, so only the > > second ends up being saved. > > > > I think it would make sense to create a single Patch() instance in this > > case with both a pull_url and the patch content (although it may be > > necessary to change the template to render such a patch correctly). Is > > there anything that would prevent that from working? > > The existing template is able to handle a patch containing both a > pull_url and a non-empty content, and the small change below will make > sure the diff is never thrown away. If there's interest in this change > I'd be happy to write a test case for it. > > > --- > apps/patchwork/bin/parsemail.py | 12 +++--------- > 1 files changed, 3 insertions(+), 9 deletions(-) > > diff --git a/apps/patchwork/bin/parsemail.py b/apps/patchwork/bin/parsemail.py > index 700cb6f..9f3561c 100755 > --- a/apps/patchwork/bin/parsemail.py > +++ b/apps/patchwork/bin/parsemail.py > @@ -183,16 +183,10 @@ def find_content(project, mail): > patch = None > comment = None > > - if patchbuf: > - mail_headers(mail) > + if pullurl or patchbuf: > name = clean_subject(mail.get('Subject'), [project.linkname]) > - patch = Patch(name = name, content = patchbuf, > - date = mail_date(mail), headers = mail_headers(mail)) > - > - if pullurl: > - name = clean_subject(mail.get('Subject'), [project.linkname]) > - patch = Patch(name = name, pull_url = pullurl, > - date = mail_date(mail), headers = mail_headers(mail)) > + patch = Patch(name = name, pull_url = pullurl, content = patchbuf, > + date = mail_date(mail), headers = mail_headers(mail)) > > if commentbuf: > if patch: > -- > 1.7.1 > If you remove two spaces in front of the last added line, the diff will be smaller and easier to review. Indentation should be a multiple of 4. With that change: Reviewed-by: Dirk Wallenstein <halsmit@t-online.de>
On Tue, 2011-02-08 at 19:40 +0100, Dirk Wallenstein wrote: > On Tue, Feb 08, 2011 at 03:22:35PM -0200, Guilherme Salgado wrote: > > On Tue, 2011-02-08 at 12:39 -0200, Guilherme Salgado wrote: > > > Hi, > > > > > > I was looking at how patchwork deals with pull requests and noticed that > > > when it parses a message containing both a pull request and a patch, it > > > will create two Patch() instances (one with the patch as its content and > > > the other without content but with a pull_url): > > > > > > if patchbuf: > > > mail_headers(mail) > > > name = clean_subject(mail.get('Subject'), [project.linkname]) > > > patch = Patch(name = name, content = patchbuf, > > > date = mail_date(mail), headers = mail_headers(mail)) > > > > > > if pullurl: > > > name = clean_subject(mail.get('Subject'), [project.linkname]) > > > patch = Patch(name = name, pull_url = pullurl, > > > date = mail_date(mail), headers = mail_headers(mail)) > > > > > > However, these are not automatically inserted in the DB, so only the > > > second ends up being saved. > > > > > > I think it would make sense to create a single Patch() instance in this > > > case with both a pull_url and the patch content (although it may be > > > necessary to change the template to render such a patch correctly). Is > > > there anything that would prevent that from working? > > > > The existing template is able to handle a patch containing both a > > pull_url and a non-empty content, and the small change below will make > > sure the diff is never thrown away. If there's interest in this change > > I'd be happy to write a test case for it. > > > > > > --- > > apps/patchwork/bin/parsemail.py | 12 +++--------- > > 1 files changed, 3 insertions(+), 9 deletions(-) > > > > diff --git a/apps/patchwork/bin/parsemail.py b/apps/patchwork/bin/parsemail.py > > index 700cb6f..9f3561c 100755 > > --- a/apps/patchwork/bin/parsemail.py > > +++ b/apps/patchwork/bin/parsemail.py > > @@ -183,16 +183,10 @@ def find_content(project, mail): > > patch = None > > comment = None > > > > - if patchbuf: > > - mail_headers(mail) > > + if pullurl or patchbuf: > > name = clean_subject(mail.get('Subject'), [project.linkname]) > > - patch = Patch(name = name, content = patchbuf, > > - date = mail_date(mail), headers = mail_headers(mail)) > > - > > - if pullurl: > > - name = clean_subject(mail.get('Subject'), [project.linkname]) > > - patch = Patch(name = name, pull_url = pullurl, > > - date = mail_date(mail), headers = mail_headers(mail)) > > + patch = Patch(name = name, pull_url = pullurl, content = patchbuf, > > + date = mail_date(mail), headers = mail_headers(mail)) > > > > if commentbuf: > > if patch: > > -- > > 1.7.1 > > > > If you remove two spaces in front of the last added line, the diff will > be smaller and easier to review. Indentation should be a multiple of 4. > With that change: > Reviewed-by: Dirk Wallenstein <halsmit@t-online.de> Thanks for the review, Dirk. I've added the two spaces to align the arguments on the second line with the ones on the first, which is the style used in PEP-8 examples, but I'm happy to revert that if this style is not appropriate for patchwork code. BTW, is there a style guide for patchwork code, or anything I could use as a reference for future changes?
On Tue, Feb 08, 2011 at 04:54:13PM -0200, Guilherme Salgado wrote: > On Tue, 2011-02-08 at 19:40 +0100, Dirk Wallenstein wrote: > > On Tue, Feb 08, 2011 at 03:22:35PM -0200, Guilherme Salgado wrote: > > > On Tue, 2011-02-08 at 12:39 -0200, Guilherme Salgado wrote: > > > > Hi, > > > > > > > > I was looking at how patchwork deals with pull requests and noticed that > > > > when it parses a message containing both a pull request and a patch, it > > > > will create two Patch() instances (one with the patch as its content and > > > > the other without content but with a pull_url): > > > > > > > > if patchbuf: > > > > mail_headers(mail) > > > > name = clean_subject(mail.get('Subject'), [project.linkname]) > > > > patch = Patch(name = name, content = patchbuf, > > > > date = mail_date(mail), headers = mail_headers(mail)) > > > > > > > > if pullurl: > > > > name = clean_subject(mail.get('Subject'), [project.linkname]) > > > > patch = Patch(name = name, pull_url = pullurl, > > > > date = mail_date(mail), headers = mail_headers(mail)) > > > > > > > > However, these are not automatically inserted in the DB, so only the > > > > second ends up being saved. > > > > > > > > I think it would make sense to create a single Patch() instance in this > > > > case with both a pull_url and the patch content (although it may be > > > > necessary to change the template to render such a patch correctly). Is > > > > there anything that would prevent that from working? > > > > > > The existing template is able to handle a patch containing both a > > > pull_url and a non-empty content, and the small change below will make > > > sure the diff is never thrown away. If there's interest in this change > > > I'd be happy to write a test case for it. > > > > > > > > > --- > > > apps/patchwork/bin/parsemail.py | 12 +++--------- > > > 1 files changed, 3 insertions(+), 9 deletions(-) > > > > > > diff --git a/apps/patchwork/bin/parsemail.py b/apps/patchwork/bin/parsemail.py > > > index 700cb6f..9f3561c 100755 > > > --- a/apps/patchwork/bin/parsemail.py > > > +++ b/apps/patchwork/bin/parsemail.py > > > @@ -183,16 +183,10 @@ def find_content(project, mail): > > > patch = None > > > comment = None > > > > > > - if patchbuf: > > > - mail_headers(mail) > > > + if pullurl or patchbuf: > > > name = clean_subject(mail.get('Subject'), [project.linkname]) > > > - patch = Patch(name = name, content = patchbuf, > > > - date = mail_date(mail), headers = mail_headers(mail)) > > > - > > > - if pullurl: > > > - name = clean_subject(mail.get('Subject'), [project.linkname]) > > > - patch = Patch(name = name, pull_url = pullurl, > > > - date = mail_date(mail), headers = mail_headers(mail)) > > > + patch = Patch(name = name, pull_url = pullurl, content = patchbuf, > > > + date = mail_date(mail), headers = mail_headers(mail)) > > > > > > if commentbuf: > > > if patch: > > > -- > > > 1.7.1 > > > > > > > If you remove two spaces in front of the last added line, the diff will > > be smaller and easier to review. Indentation should be a multiple of 4. > > With that change: > > Reviewed-by: Dirk Wallenstein <halsmit@t-online.de> > > Thanks for the review, Dirk. > > I've added the two spaces to align the arguments on the second line with > the ones on the first, which is the style used in PEP-8 examples, but Oh, indeed. Haven't noticed that. I always use two additional levels. However, I think, whitespace changes on lines that don't change otherwise are best placed in separate commits -- easier to review.
Hi Guilherme, > Oh, indeed. Haven't noticed that. I always use two additional levels. > However, I think, whitespace changes on lines that don't change > otherwise are best placed in separate commits -- easier to review. For changes like this, I'd prefer to keep it consistent with the existing formatting. We can revisit things later if it'd help though. Also, as Dirk has suggested, would you be able to send your patches with something like git-format-patch and git-send-email? Having a proper changelog entry helps me enormously, as does including test cases where appropriate (don't worry about a testcase for this, but the pull request change could definitely use one). Cheers, Jeremy
On Fri, 2011-02-11 at 11:37 +0800, Jeremy Kerr wrote: > Hi Guilherme, > > > Oh, indeed. Haven't noticed that. I always use two additional levels. > > However, I think, whitespace changes on lines that don't change > > otherwise are best placed in separate commits -- easier to review. > > For changes like this, I'd prefer to keep it consistent with the existing > formatting. We can revisit things later if it'd help though. Ok > > Also, as Dirk has suggested, would you be able to send your patches with > something like git-format-patch and git-send-email? Having a proper changelog > entry helps me enormously, as does including test cases where appropriate I've just done that, please let me know if it has everything you need. > (don't worry about a testcase for this, but the pull request change could > definitely use one). Well, this is the pull request change, so the patch I just sent includes a testcase. :) Cheers,
diff --git a/apps/patchwork/bin/parsemail.py b/apps/patchwork/bin/parsemail.py index 700cb6f..9f3561c 100755 --- a/apps/patchwork/bin/parsemail.py +++ b/apps/patchwork/bin/parsemail.py @@ -183,16 +183,10 @@ def find_content(project, mail): patch = None comment = None - if patchbuf: - mail_headers(mail) + if pullurl or patchbuf: name = clean_subject(mail.get('Subject'), [project.linkname]) - patch = Patch(name = name, content = patchbuf, - date = mail_date(mail), headers = mail_headers(mail)) - - if pullurl: - name = clean_subject(mail.get('Subject'), [project.linkname]) - patch = Patch(name = name, pull_url = pullurl, - date = mail_date(mail), headers = mail_headers(mail)) + patch = Patch(name = name, pull_url = pullurl, content = patchbuf, + date = mail_date(mail), headers = mail_headers(mail)) if commentbuf: if patch: