Skip to content

Conversation

@djkirkham
Copy link
Contributor

Closes #2654

@djkirkham djkirkham requested review from DPeterK, corinnebosley and pp-mo and removed request for pp-mo October 11, 2017 16:20
Copy link
Member

@corinnebosley corinnebosley left a comment

Choose a reason for hiding this comment

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

Looks good to me. Pending tests passing, I would be happy to merge.

@QuLogic QuLogic added this to the v2.0 milestone Oct 12, 2017
Copy link
Member

@DPeterK DPeterK left a comment

Choose a reason for hiding this comment

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

@djkirkham just a small thing from me, otherwise looking good! 👍

"'iris_sample_data' package.")
warn_deprecated(wmsg)
target = os.path.join(iris.config.SAMPLE_DATA_DIR, target)
raise ValueError("Please install the 'iris_sample_data' package to "
Copy link
Member

Choose a reason for hiding this comment

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

It's finicky I know, but could this be an ImportError? Seems to make more sense.

You could even then make this a try-except block then.

class TestIrisSampleDataMissing(tests.IrisTest):
def test_no_iris_sample_data(self):
self.patch('iris.iris_sample_data', None)
with self.assertRaisesRegexp(ValueError, 'Please install'):
Copy link
Member

Choose a reason for hiding this comment

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

If you choose to change the error class above you'll need to change it here too.

warn_deprecated(wmsg)
target = os.path.join(iris.config.SAMPLE_DATA_DIR, target)
raise ImportError("Please install the 'iris_sample_data' package to "
"access sample data.")
Copy link
Member

Choose a reason for hiding this comment

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

Dah! This line is unindented by just one character but it's causing a whole load of test failures 😢

@djkirkham djkirkham force-pushed the deprecate_sample_data_dir branch from 66d2b8d to dda2af5 Compare October 12, 2017 11:08
@corinnebosley corinnebosley merged commit 27d833e into SciTools:master Oct 12, 2017
@djkirkham djkirkham deleted the deprecate_sample_data_dir branch October 26, 2017 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants