Skip to content

Conversation

@QuLogic
Copy link
Member

@QuLogic QuLogic commented Oct 10, 2016

The pep8 package was pinned in #1558. To be able to run on the latest packages, this pinning needs to be removed. So these style issues need to be corrected as well.

On a related note, when should we depend on pycodestyle instead of pep8?

@QuLogic QuLogic added this to the v1.11 milestone Oct 10, 2016
@marqh
Copy link
Member

marqh commented Oct 12, 2016

Hi @QuLogic

i think this is a positive step, i am in favour

I had not realised that pep8 explicitly pushed against lambda functions in the way this PR suggests

have you considered whether we should adopt this pattern within the user guide? e.g.
http://scitools.org.uk/iris/docs/latest/userguide/loading_iris_cubes.html#constrained-loading
(scroll down a bit, to:
As with many of the examples later in this documentation, the simple function above can be conveniently written as a lambda function on a single line:
)

or is the key pep8 suggestion that multi line lamda functions are naff, rather than all lambda functions?

@ajdawson
Copy link
Member

is the key pep8 suggestion that multi line lamda functions are naff, rather than all lambda functions?

PEP8 suggests you prefer a def statement instead of binding a lambda to an identifier, it doesn't matter what the form of the lambda is, it just shouldn't be bound to a name.

have you considered whether we should adopt this pattern within the user guide?

The user guide is suggesting ways of doing things to end users, and as such I don't think we need to be strict about following a style document there (it is the end user's decision what style to follow, if any).

# A dramatic speedup can be had if we don't have bounds.
if coord.has_bounds():
call_func = lambda cell: cell in desired_values
def call_func(cell):
Copy link
Member

Choose a reason for hiding this comment

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

This could be a one-liner like the original:

def call_func(cell): return cell in desired_values

Any strong preferences?

Copy link
Member

Choose a reason for hiding this comment

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

i find reading over multiple lines clearer than having a def and func on one line, I prefer @QuLogic 's current, I think

@marqh
Copy link
Member

marqh commented Oct 12, 2016

all the individual code changes seem positive or neutral to me

the overall aim of unpinning is clearly better

I see no reason not to merge

last call for caution.....

@marqh
Copy link
Member

marqh commented Oct 13, 2016

Hi @QuLogic

this is great stuff, I am fully on board.

there has been a lot of activity recently, centred on #2124, which has just been merged (woo!)

please may you rebase onto master so that the tests rerun, in case there is any pep8 fallout which we have missed

thank you
mark

@marqh
Copy link
Member

marqh commented Oct 13, 2016

please may you also add a change to
https://github.com/SciTools/iris/blob/master/INSTALL#L135
removing the *

thank you

This warning is not a major problem, but most of the code is already
styled in this manner.
This is "E731 do not assign a lambda expression, use a def" in pep8; in
many cases, it is quite right to complain, especially with multi-line
lambdas.
Due to conditional imports and such, this happens a few times and is
annoying to ignore at each location.
@marqh
Copy link
Member

marqh commented Oct 14, 2016

excellent, many thanks @QuLogic

@marqh marqh merged commit baef126 into SciTools:master Oct 14, 2016
@QuLogic QuLogic deleted the pep8-updates branch October 14, 2016 12:24
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.

3 participants