Skip to content

Change behaviour of 'error' level #2881

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
DPeterK merged 3 commits into from
Oct 31, 2017
Merged

Change behaviour of 'error' level #2881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
902663e
Change behaviour of 'error' level
djkirkham Oct 27, 2017
fd6202e
Change Future netcdf_no_unlimited tests
djkirkham Oct 30, 2017
b0b3f71
Fix tests testing the wrong thing
djkirkham Oct 30, 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
2 changes: 1 addition & 1 deletion lib/iris/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ def __repr__(self):
def __setattr__(self, name, value):
if name in self.deprecated_options:
level = self.deprecated_options[name]
if level == 'error':
if level == 'error' and not value:
Copy link
Member

@DPeterK DPeterK Oct 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand the purpose of this change. If __setattr__ has been called, there will always be a value. For example:

>>> import iris
>>> # Specifically enable deprecated behaviour.
>>> iris.FUTURE.cell_datetime_objects = False  # key=cell_datetime_objects; value=False
DeprecationWarning...
>>>

This is why the test is failing, as it is testing precisely this behaviour, is being passed a value and is thus failing as this if block evaluates to False.

Copy link
Contributor Author

@djkirkham djkirkham Oct 30, 2017

Choose a reason for hiding this comment

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

not False == True...

Copy link
Contributor Author

@djkirkham djkirkham Oct 30, 2017

Choose a reason for hiding this comment

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

I should have said not True == False which is the case when we don't want this if block to run.

Copy link
Member

@DPeterK DPeterK Oct 30, 2017

Choose a reason for hiding this comment

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

Of course it is. That always catches me out.

emsg = ("setting the 'Future' property {prop!r} has been "
"deprecated to be removed in a future release, and "
"deprecated {prop!r} behaviour has been removed. "
Expand Down
34 changes: 21 additions & 13 deletions lib/iris/tests/unit/test_Future.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,22 +43,30 @@ def test_invalid_attribute(self):
with self.assertRaises(AttributeError):
future.numberwang = 7

def test_invalid_netcdf_promote_attributes(self):
def test_netcdf_promote(self):
future = Future()
states = [True, False]
exp_emsg = "deprecated 'netcdf_promote' behaviour has been removed"
for state in states:
with self.assertRaisesRegexp(AttributeError, exp_emsg):
future.netcdf_promote = state
exp_emsg = "'Future' property 'netcdf_promote' is deprecated"
with self.assertWarnsRegexp(exp_emsg):
future.netcdf_promote = True

def test_invalid_netcdf_no_unlimited_attributes(self):
def test_invalid_netcdf_promote(self):
future = Future()
states = [True, False]
exp_emsg =\
"deprecated 'netcdf_no_unlimited' behaviour has been removed"
for state in states:
with self.assertRaisesRegexp(AttributeError, exp_emsg):
future.netcdf_no_unlimited = state
exp_emsg = "'Future' property 'netcdf_promote' has been deprecated"
Copy link
Member

@DPeterK DPeterK Oct 30, 2017

Choose a reason for hiding this comment

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

Hold on, the Future property being tested and the test name no longer match ⚠️

netcdf_no_unlimited != netcdf_promote

Copy link
Contributor Author

@djkirkham djkirkham Oct 30, 2017

Choose a reason for hiding this comment

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

This one's fine, well spotted on the other two though

Copy link
Member

@DPeterK DPeterK Oct 31, 2017

Choose a reason for hiding this comment

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

Copy-paste mistakes are my favourite mistakes to make 😉

with self.assertRaisesRegexp(AttributeError, exp_emsg):
future.netcdf_promote = False

def test_netcdf_no_unlimited(self):
future = Future()
exp_emsg = "'Future' property 'netcdf_no_unlimited' is deprecated"
with self.assertWarnsRegexp(exp_emsg):
future.netcdf_no_unlimited = True

def test_invalid_netcdf_no_unlimited(self):
future = Future()
exp_emsg = \
"'Future' property 'netcdf_no_unlimited' has been deprecated"
with self.assertRaisesRegexp(AttributeError, exp_emsg):
future.netcdf_no_unlimited = False

def test_cell_datetime_objects(self):
future = Future()
Expand Down