Skip to content

Conversation

@pp-mo
Copy link
Member

@pp-mo pp-mo commented Dec 11, 2012

Addresses #86

For now:

  • mechanism to switch support between allow/disallow/deprecate.
  • default is now 'deprecate', which gives a warning
  • tests for all-options, and allow-only (which then fails if setting is 'disallow')

Future Intentions:

  • when ready to disallow, remove switch + leave disabling code only.

@pp-mo
Copy link
Member Author

pp-mo commented Dec 11, 2012

@pelson did you suggest there may be specific problems with this?
Currently at least, passes tests, doctest + extests.
What else might need checking?

@rhattersley
Copy link
Member

It looks a trifle over-engineered...?

@pp-mo
Copy link
Member Author

pp-mo commented Dec 12, 2012

@rhattersley It looks a trifle over-engineered...?

Agreed. It was really just for testing that I discovered it was handy to make it configurable. The extra stuff should certainly all be removed once it has been 'deprecated' for a while and we switch to 'disallow'.
It is easy to deliver any of ...

  • reduce to the minimum for 'deprecated' behaviour
  • remove the config interface, but retain the internal control setting
  • leave as-is

What would you prefer right now?

@rhattersley
Copy link
Member

What would you prefer right now?

Personally I'd go for option one - "reduce to the minimum".

 : test included
@pp-mo
Copy link
Member Author

pp-mo commented Dec 13, 2012

You might question my use of warnings.catch_warnings, as it is apparently not thread-safe ?
-- see : https://groups.google.com/forum/?hl=en-GB&fromgroups=#!topic/scitools-iris-dev/X3HgTXeoTRk

If you think this is plainly unwise, I could probably use Mock instead ?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer:

with warnings.catch_warnings():
    warnings.simplefilter("error")
    with self.assertRaises(DeprecationWarning):
        for subcube in cube:
            pass

as it is standalone (it does not create cube_iterator for use by later test code) and it's a little clearer (to me at least) than the generator expression. It's still not thread, but I can live with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed that is much neater @esc24 -- really rather lovely!
I am still tempted to use subcubes = list(cube) for the iter test expression, but maybe that is not obvious enough?

 : clearer test code
 : don't use DeprecationWarning
@pp-mo
Copy link
Member Author

pp-mo commented Dec 14, 2012

Improved by @esc24 suggestion.
Also removed the use of the 'DeprecationWarning' category, because I found this is set to 'ignore' by default since python2.7 -- see http://docs.python.org/2/library/warnings.html#warning-categories

@pp-mo
Copy link
Member Author

pp-mo commented Dec 14, 2012

I think the testcode is now simpler+clearer.
But I am still "tempted to use subcubes = list(cube)" as a neater equivalent for "subcubes = [subcube for subcube in cube]" -- which effectively is now done twice in the test code.
@esc24 does this seem a more obscure version to you?

@rhattersley
Copy link
Member

You might question my use of warnings.catch_warnings, as it is apparently not thread-safe ?

I don't think we need to worry about it. We run the tests in a single-threaded mode. Even nosetests uses multiple processes, not threads.

lib/iris/cube.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Docstring is not needed IMHO, a single comment would suffice.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

Changing to a comment would also fix the missing blank line from the docstring. ;-)

@rhattersley
Copy link
Member

Thanks - good to go. :-)

rhattersley added a commit that referenced this pull request Jan 18, 2013
@rhattersley rhattersley merged commit cdad5b8 into SciTools:master Jan 18, 2013
@pp-mo pp-mo deleted the no_cube_iter branch March 4, 2013 16:42
@pp-mo pp-mo mentioned this pull request Feb 14, 2020
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