Skip to content

Correct STASH regular expression #2638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
djkirkham merged 10 commits into from
Oct 11, 2017
Merged

Correct STASH regular expression #2638

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8e02c7a
Correct STASH regular expression
ehogan Jul 5, 2017
44b9c79
Revert "Correct STASH regular expression"
ehogan Jul 5, 2017
bfa7b88
Add test to show failure of current STASH regular expression
ehogan Jul 5, 2017
58bf91f
Correct STASH regular expression
ehogan Jul 6, 2017
e97a314
Update STASH-related tests accordingly
ehogan Jul 6, 2017
506c6d3
Restore a irregular STASH test
ehogan Jul 6, 2017
38d91c0
Update regular expression to handle question marks for the model number
ehogan Jul 19, 2017
fe3fd3c
Changes in response to review: update format for illegal_stash tests,…
ehogan Sep 4, 2017
5317a5a
Relax regex to allow varying lengths and reinstate tests
djkirkham Sep 27, 2017
84468cb
Merge pull request #1 from djkirkham/correct_stash_regex
ehogan Oct 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/iris/fileformats/pp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,8 @@ def from_msi(msi):
if not isinstance(msi, six.string_types):
raise TypeError('Expected STASH code MSI string, got %r' % (msi,))

msi_match = re.match('^\s*m(.*)s(.*)i(.*)\s*$', msi, re.IGNORECASE)
msi_match = re.match('^\s*m(\d{2}|\?{2})s(\d{2})i(\d{3})\s*$', msi,
re.IGNORECASE)

if msi_match is None:
raise ValueError('Expected STASH code MSI string "mXXsXXiXXX", '
Expand Down
80 changes: 49 additions & 31 deletions lib/iris/tests/test_pp_stash.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# (C) British Crown Copyright 2010 - 2015, Met Office
# (C) British Crown Copyright 2010 - 2017, Met Office
#
# This file is part of Iris.
#
Expand Down Expand Up @@ -59,45 +59,17 @@ def test_stash_against_str(self):
self.assertNotEqual('m02s03i004', iris.fileformats.pp.STASH(1, 2, 3))

def test_irregular_stash_str(self):
self.assertEqual(iris.fileformats.pp.STASH(1, 2, 3), 'm01s02i0000000003')
self.assertEqual(iris.fileformats.pp.STASH(1, 2, 3), 'm01s02i3')
self.assertEqual(iris.fileformats.pp.STASH(1, 2, 3), 'm01s2i3')
self.assertEqual(iris.fileformats.pp.STASH(1, 2, 3), 'm1s2i3')

self.assertEqual('m01s02i0000000003', iris.fileformats.pp.STASH(1, 2, 3))
self.assertEqual('m01s02i3', iris.fileformats.pp.STASH(1, 2, 3))
self.assertEqual('m01s2i3', iris.fileformats.pp.STASH(1, 2, 3))
self.assertEqual('m1s2i3', iris.fileformats.pp.STASH(1, 2, 3))

self.assertNotEqual(iris.fileformats.pp.STASH(2, 3, 4), 'm01s02i0000000003')
self.assertNotEqual(iris.fileformats.pp.STASH(2, 3, 4), 'm01s02i3')
self.assertNotEqual(iris.fileformats.pp.STASH(2, 3, 4), 'm01s2i3')
self.assertNotEqual(iris.fileformats.pp.STASH(2, 3, 4), 'm1s2i3')

self.assertNotEqual('m01s02i0000000003', iris.fileformats.pp.STASH(2, 3, 4))
self.assertNotEqual('m01s02i3', iris.fileformats.pp.STASH(2, 3, 4))
self.assertNotEqual('m01s2i3', iris.fileformats.pp.STASH(2, 3, 4))
self.assertNotEqual('m1s2i3', iris.fileformats.pp.STASH(2, 3, 4))

self.assertEqual(iris.fileformats.pp.STASH.from_msi('M01s02i003'), 'm01s02i003')
self.assertEqual('m01s02i003', iris.fileformats.pp.STASH.from_msi('M01s02i003'))

def test_illegal_stash_str_range(self):
self.assertEqual(iris.fileformats.pp.STASH(0, 2, 3), 'm00s02i003')
self.assertEqual(iris.fileformats.pp.STASH(0, 2, 3), 'm??s02i003')
self.assertNotEqual(iris.fileformats.pp.STASH(0, 2, 3), 'm01s02i003')
self.assertEqual('m00s02i003', iris.fileformats.pp.STASH(0, 2, 3))
self.assertEqual('m??s02i003', iris.fileformats.pp.STASH(0, 2, 3))
self.assertNotEqual('m01s02i003', iris.fileformats.pp.STASH(0, 2, 3))

self.assertEqual(iris.fileformats.pp.STASH(0, 2, 3), 'm??s02i003')
self.assertEqual(iris.fileformats.pp.STASH(0, 2, 3), 'm00s02i003')
self.assertEqual('m??s02i003', iris.fileformats.pp.STASH(0, 2, 3))
self.assertEqual('m00s02i003', iris.fileformats.pp.STASH(0, 2, 3))

self.assertEqual(iris.fileformats.pp.STASH(100, 2, 3), 'm??s02i003')
self.assertEqual(iris.fileformats.pp.STASH(100, 2, 3), 'm100s02i003')
self.assertEqual('m??s02i003', iris.fileformats.pp.STASH(100, 2, 3))
self.assertEqual('m100s02i003', iris.fileformats.pp.STASH(100, 2, 3))

def test_illegal_stash_stash_range(self):
self.assertEqual(iris.fileformats.pp.STASH(0, 2, 3), iris.fileformats.pp.STASH(0, 2, 3))
self.assertEqual(iris.fileformats.pp.STASH(100, 2, 3), iris.fileformats.pp.STASH(100, 2, 3))
Expand All @@ -109,11 +81,57 @@ def test_illegal_stash_format(self):
with self.assertRaises(ValueError):
self.assertEqual('abc', iris.fileformats.pp.STASH(1, 2, 3))

with self.assertRaises(ValueError):
self.assertEqual(iris.fileformats.pp.STASH(1, 2, 3), 'mlotstmin')
with self.assertRaises(ValueError):

@DPeterK DPeterK Aug 30, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I realise that this isn't directly your problem as you've copied an existing pattern, it's just the pattern you've copied is quite unusual... What this is doing is using one of the unittest assertions to catch an error raised by another unittest assertion 😱

I would prefer to see something like the following in all of these cases:

test_val = 'mlotstmin'
exp_emsg = 'Expected STASH code .* {!r}'.format(test_val)
with self.assertRaisesRegexp(ValueError, exp_emsg):
    test_val == iris.fileformats.pp.STASH(1, 2, 3)

(give or take - I haven't tested this code!)

There are two changes here:

  • I've removed the indented self.assertEqual and replaced it with a pure Python equality check
  • I've replaced self.assertRaises with the preferred (and more robust) self.assertRaisesRegexp and added the expected regular expression failure. This tests that the right error has been raised, and that this test isn't throwing a false positive failure.

self.assertEqual('mlotstmin', iris.fileformats.pp.STASH(1, 2, 3))

with self.assertRaises(ValueError):
self.assertEqual(iris.fileformats.pp.STASH(1, 2, 3), 'm01s02003')
with self.assertRaises(ValueError):
self.assertEqual('m01s02003', iris.fileformats.pp.STASH(1, 2, 3))

with self.assertRaises(ValueError):
self.assertEqual(iris.fileformats.pp.STASH(100, 2, 3), 'm100s02i003')
with self.assertRaises(ValueError):
self.assertEqual('m100s02i003', iris.fileformats.pp.STASH(100, 2, 3))

with self.assertRaises(ValueError):
self.assertEqual(iris.fileformats.pp.STASH(1, 2, 3), 'm01s02i0000000003')
with self.assertRaises(ValueError):
self.assertEqual('m01s02i0000000003', iris.fileformats.pp.STASH(1, 2, 3))
with self.assertRaises(ValueError):
self.assertNotEqual(iris.fileformats.pp.STASH(2, 3, 4), 'm01s02i0000000003')
with self.assertRaises(ValueError):
self.assertNotEqual('m01s02i0000000003', iris.fileformats.pp.STASH(2, 3, 4))

with self.assertRaises(ValueError):
self.assertEqual(iris.fileformats.pp.STASH(1, 2, 3), 'm01s02i3')
with self.assertRaises(ValueError):
self.assertEqual('m01s02i3', iris.fileformats.pp.STASH(1, 2, 3))
with self.assertRaises(ValueError):
self.assertNotEqual(iris.fileformats.pp.STASH(2, 3, 4), 'm01s02i3')
with self.assertRaises(ValueError):
self.assertNotEqual('m01s02i3', iris.fileformats.pp.STASH(2, 3, 4))

with self.assertRaises(ValueError):
self.assertEqual(iris.fileformats.pp.STASH(1, 2, 3), 'm01s2i3')
with self.assertRaises(ValueError):
self.assertEqual('m01s2i3', iris.fileformats.pp.STASH(1, 2, 3))
with self.assertRaises(ValueError):
self.assertNotEqual(iris.fileformats.pp.STASH(2, 3, 4), 'm01s2i3')
with self.assertRaises(ValueError):
self.assertNotEqual('m01s2i3', iris.fileformats.pp.STASH(2, 3, 4))

with self.assertRaises(ValueError):
self.assertEqual(iris.fileformats.pp.STASH(1, 2, 3), 'm1s2i3')
with self.assertRaises(ValueError):
self.assertEqual('m1s2i3', iris.fileformats.pp.STASH(1, 2, 3))
with self.assertRaises(ValueError):
self.assertNotEqual(iris.fileformats.pp.STASH(2, 3, 4), 'm1s2i3')
with self.assertRaises(ValueError):
self.assertNotEqual('m1s2i3', iris.fileformats.pp.STASH(2, 3, 4))

def test_illegal_stash_type(self):
with self.assertRaises(TypeError):
self.assertEqual(iris.fileformats.pp.STASH.from_msi(102003), 'm01s02i003')
Expand Down