Skip to content

Conversation

@LukeC92
Copy link
Contributor

@LukeC92 LukeC92 commented Oct 11, 2017

This is a continuation of @dkillick 's Pull Request #2697 . As well as removing rule logging config items and all rule logging functionality from fileformats/rules.py I also removed test_verbose_fileformat_rules_logging.py which now seems redundant, and a referece to rule logs in pp.save_pairs_from_cube .

Closes #2655

@QuLogic QuLogic added this to the v2.0 milestone Oct 12, 2017
#################
# Logging options
_LOGGING_SECTION = 'Logging'

Copy link
Member

Choose a reason for hiding this comment

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

@LukeC92 @dkillick @pelson @pp-mo @djkirkham
I know that this entire set of logging options is not deprecated, but while we are working towards Iris 2.0 and have also just implemented Python logging (with a view to further development), I really think we should take a long hard look at whether we still need this and should absolutely remove it if we don't.

Copy link
Member

Choose a reason for hiding this comment

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

Yep. import_logger can be removed long-term.
Definitely a separate PR, but I'd probably support it happening for 2.0 without a deprecation warning. We will have to document the breaking change though (and think about the impact for people who have a logging section in their config - would it now break if we removed it, or just be ignored silently?).

All questions to be answered outside of this PR.

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.

@LukeC92 Generally I am happy with these changes, and so are the tests, which is good. I would like a conversation with some members of the team about the remaining logging options, but if we follow all the rules that will not affect this PR, so I am happy to approve it.

Good job Luke.

@pelson
Copy link
Member

pelson commented Oct 13, 2017

My favourite kind of PR - one that makes the codebase smaller & tidier.
Thanks @LukeC92!

@pelson pelson merged commit f7e103b into SciTools:master Oct 13, 2017
@LukeC92
Copy link
Contributor Author

LukeC92 commented Oct 18, 2017

I've started writing up the "What's New" for this pull request but I am not sure what the best way is to represent all of these changes.

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.

5 participants