Skip to content
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.

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

Introduce A006 for lambda shadowing #123

Merged
merged 6 commits into from
Apr 1, 2024

Conversation

cielavenir
Copy link
Contributor

I added this to run_tests.py:

def test_lambda_argument_message():
    source = 'takefirst = lambda list: list[0]'
    check_code(source, 'A002')

Then I see this:

>       assert len(return_statements) == len(expected_codes)
E       AssertionError: assert 0 == 1
E        +  where 0 = len([])
E        +  and   1 = len(['A002'])

This MR tries to fix it.

Reproduced in main/Python3, 1.5/Python3 and 1.5/Python2.

/cc @felixvd @ntohge

@gforcada
Copy link
Owner

gforcada commented Mar 18, 2024

Thanks, for taking the time to write the PR! 🙇🏾 😄

Though, I'm not sure if lambda's are really a use case for flake8-builtins 🤔

I mean, if you do:

my_list = list()
my_list.append(1)
my_list.append(2)
my_list.append(3)

list = [1,2,3]

another_list = list()

This last line breaks due to having reassigned the builtin list to an actual list.

But if we do the same with a lambda:

one_list = list()
one_list.append(1)
one_list.append(2)
one_list.append(3)

my_lambda = lambda list: list[0]

my_lambda(one_list) # returns 1

second_list = list()
second_list.append(4)
second_list.append(5)
second_list.append(6)

my_lambda(second_list) # returns 4

This still works... let me think a bit more about it before deciding 🤔

@felixvd
Copy link
Contributor

felixvd commented Mar 19, 2024

my_lambda = lambda list: list[0]

I agree this isn't the same level of risk as other A002 errors , but it doesn't exactly look like great practice either.

@gforcada
Copy link
Owner

Indeed, but then, maybe we could use a new error code A005 for it? 🤔 It means a bit more work though 😕

@cielavenir cielavenir changed the title Raise A002 for lambda Introduce A005 for lambda shadowing Mar 20, 2024
@cielavenir
Copy link
Contributor Author

@gforcada done

(sorry that check_function_definition and check_lambda_definition have duplicated code)

@felixvd
Copy link
Contributor

felixvd commented Mar 26, 2024

@cielavenir Let's update the Readme too:

A005:
  A lambda argument is shadowing a Python builtin.

@cielavenir
Copy link
Contributor Author

@felixvd done.

@gforcada
Copy link
Owner

🙈 sorry! I just merged another contribution that added a A005, we should turn this into a A006. Would you mind updating the code? I can try myself to do it otherwise, though, I'm a bit busy as of late 😕

@cielavenir cielavenir changed the title Introduce A005 for lambda shadowing Introduce A006 for lambda shadowing Mar 30, 2024
@cielavenir
Copy link
Contributor Author

@gforcada merged main

@gforcada
Copy link
Owner

@cielavenir one last request, sorry! 🙏🏾 could you add a line on CHANGES.rst as well? 🍀

With this it will be ready to get merged 😊

@cielavenir
Copy link
Contributor Author

@gforcada done

@gforcada
Copy link
Owner

gforcada commented Apr 1, 2024

Thanks!! 😄

The date change was not needed, I will revert it myself after the merge.

@gforcada gforcada merged commit 116ce7b into gforcada:main Apr 1, 2024
6 of 7 checks passed
@gforcada
Copy link
Owner

gforcada commented Apr 1, 2024

... and flake8-builtins==2.4.0 is released! 🎉

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