| Message ID | 1439822766-7723-1-git-send-email-jose.a.lamego@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On Mon, Aug 17, 2015 at 09:46:06AM -0500, Jose Lamego wrote: > Avoids some email patch notifications to be > wrongly skipped when the commit's descriptor string > includes or is missing a character/space from the > expected format. > An example of a missed email patch notification > can be found in [1]. > > [1] http://patchwork.openembedded.org/patch/96385/ > > Signed-off-by: Jose Lamego <jose.a.lamego@linux.intel.com> There's already a bit of coverage of that function in tests/test_patchparser.py, would you mind adding coverage for the case you're fixing? requiring unit tests is really the best way to make sure we continue building up the test suite with cases users care about.
On Mon, Aug 17, 2015 at 09:46:06AM -0500, Jose Lamego wrote: > Avoids some email patch notifications to be > wrongly skipped when the commit's descriptor string > includes or is missing a character/space from the > expected format. > An example of a missed email patch notification > can be found in [1]. > > [1] http://patchwork.openembedded.org/patch/96385/ > > Signed-off-by: Jose Lamego <jose.a.lamego@linux.intel.com> > > --- > patchwork/bin/parsemail.py | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py > index f2b10bd..902e554 100755 > --- a/patchwork/bin/parsemail.py > +++ b/patchwork/bin/parsemail.py > @@ -141,8 +141,8 @@ def mail_headers(mail): > > def find_pull_request(content): > git_re = re.compile('^The following changes since commit.*' + > - '^are available in the git repository at:\n' > - '^\s*([\S]+://[^\n]+)$', > + '^are available in the git repository at.*: The string literal isn't closed making it unlikely that patch is working :) Running the unit tests caught it: ImportError: Failed to import test module: patchwork.tests.test_patchparser Traceback (most recent call last): File "/usr/lib/python2.7/unittest/loader.py", line 254, in _find_tests module = self._get_module_from_name(name) File "/usr/lib/python2.7/unittest/loader.py", line 232, in _get_module_from_name __import__(name) File "/var/lib/jenkins/jobs/patchwork-github/workspace/patchwork/tests/test_patchparser.py", line 40, in <module> from patchwork.bin.parsemail import find_content, find_author, find_project, \ File "/var/lib/jenkins/jobs/patchwork-github/workspace/patchwork/bin/parsemail.py", line 144 '^are available in the git repository at.*: ^ SyntaxError: EOL while scanning string literal
Sent patch v2 with missing character. I still need to work on the coverage. Jose On 08/20/2015 10:10 AM, Damien Lespiau wrote: > On Mon, Aug 17, 2015 at 09:46:06AM -0500, Jose Lamego wrote: >> Avoids some email patch notifications to be >> wrongly skipped when the commit's descriptor string >> includes or is missing a character/space from the >> expected format. >> An example of a missed email patch notification >> can be found in [1]. >> >> [1] http://patchwork.openembedded.org/patch/96385/ >> >> Signed-off-by: Jose Lamego <jose.a.lamego@linux.intel.com> >> >> --- >> patchwork/bin/parsemail.py | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py >> index f2b10bd..902e554 100755 >> --- a/patchwork/bin/parsemail.py >> +++ b/patchwork/bin/parsemail.py >> @@ -141,8 +141,8 @@ def mail_headers(mail): >> >> def find_pull_request(content): >> git_re = re.compile('^The following changes since commit.*' + >> - '^are available in the git repository at:\n' >> - '^\s*([\S]+://[^\n]+)$', >> + '^are available in the git repository at.*: > The string literal isn't closed making it unlikely that patch is working :) > > Running the unit tests caught it: > > ImportError: Failed to import test module: patchwork.tests.test_patchparser > Traceback (most recent call last): > File "/usr/lib/python2.7/unittest/loader.py", line 254, in _find_tests > module = self._get_module_from_name(name) > File "/usr/lib/python2.7/unittest/loader.py", line 232, in _get_module_from_name > __import__(name) > File "/var/lib/jenkins/jobs/patchwork-github/workspace/patchwork/tests/test_patchparser.py", line 40, in <module> > from patchwork.bin.parsemail import find_content, find_author, find_project, \ > File "/var/lib/jenkins/jobs/patchwork-github/workspace/patchwork/bin/parsemail.py", line 144 > '^are available in the git repository at.*: > ^ > SyntaxError: EOL while scanning string literal >
On Mon, Aug 24, 2015 at 10:20:34AM -0500, Jose Lamego wrote:
> Sent patch v2 with missing character. I still need to work on the coverage.
Thanks. I believe it's good practice to wait until the test coverage is
in place to consider merging the change. When it touches to mail parsing
it's fairly easy to do so. Of course, we need to make sure the pull_url
still contains something valid.
On Tue, Aug 25, 2015 at 11:27:45AM +0100, Damien Lespiau wrote: > On Mon, Aug 24, 2015 at 10:20:34AM -0500, Jose Lamego wrote: > > Sent patch v2 with missing character. I still need to work on the coverage. > > Thanks. I believe it's good practice to wait until the test coverage is > in place to consider merging the change. When it touches to mail parsing > it's fairly easy to do so. Of course, we need to make sure the pull_url > still contains something valid. Any news on the test case side? I'd rather not pull this in without test coverage. Thanks,
Hi Damien, It was a good call on waiting for the test. The v2 patch made the "testGitPullWithDiff" to fail. I'm sending separately a v3 patch that does the job without undesired effects along with the added test "testGitPullWithBranch". I'm moving the status of this v2 to superseded state. Thanks, On 17/09/15 12:04, Damien Lespiau wrote: > On Tue, Aug 25, 2015 at 11:27:45AM +0100, Damien Lespiau wrote: >> On Mon, Aug 24, 2015 at 10:20:34AM -0500, Jose Lamego wrote: >>> Sent patch v2 with missing character. I still need to work on the coverage. >> Thanks. I believe it's good practice to wait until the test coverage is >> in place to consider merging the change. When it touches to mail parsing >> it's fairly easy to do so. Of course, we need to make sure the pull_url >> still contains something valid. > Any news on the test case side? I'd rather not pull this in without > test coverage. > > Thanks, >
diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py index f2b10bd..902e554 100755 --- a/patchwork/bin/parsemail.py +++ b/patchwork/bin/parsemail.py @@ -141,8 +141,8 @@ def mail_headers(mail): def find_pull_request(content): git_re = re.compile('^The following changes since commit.*' + - '^are available in the git repository at:\n' - '^\s*([\S]+://[^\n]+)$', + '^are available in the git repository at.*: + '^\s*([\S]+://[^\n]+)', re.DOTALL | re.MULTILINE) match = git_re.search(content) if match:
Avoids some email patch notifications to be wrongly skipped when the commit's descriptor string includes or is missing a character/space from the expected format. An example of a missed email patch notification can be found in [1]. [1] http://patchwork.openembedded.org/patch/96385/ Signed-off-by: Jose Lamego <jose.a.lamego@linux.intel.com> --- patchwork/bin/parsemail.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)