Skip to content

avoid overwritting pr_nr in post_pr_test_report for reports with --include-easyblocks-from-pr#3724

Merged
migueldiascosta merged 1 commit intoeasybuilders:developfrom
lexming:fix-upload-test
Jun 5, 2021
Merged

avoid overwritting pr_nr in post_pr_test_report for reports with --include-easyblocks-from-pr#3724
migueldiascosta merged 1 commit intoeasybuilders:developfrom
lexming:fix-upload-test

Conversation

@lexming
Copy link
Copy Markdown
Contributor

@lexming lexming commented Jun 5, 2021

Fixes issue discussed in easybuilders/easybuild-easyconfigs#9902 (comment)

The problem is that this pr_nr variable in the list comprehension is overwriting the pr_nr variable in the post_pr_test_report scope.

Copy link
Copy Markdown
Member

@migueldiascosta migueldiascosta left a comment

Choose a reason for hiding this comment

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

lgtm!

@migueldiascosta migueldiascosta added this to the next release (4.4.1) milestone Jun 5, 2021
@migueldiascosta
Copy link
Copy Markdown
Member

Going in, thanks @lexming!

@migueldiascosta migueldiascosta merged commit 22b9603 into easybuilders:develop Jun 5, 2021
@boegel boegel changed the title avoid overwritting pr_nr in post_pr_test_report for reports with --in… avoid overwritting pr_nr in post_pr_test_report for reports with --include-easyblocks-from-pr Jun 5, 2021
if build_option('include_easyblocks_from_pr'):
if repo_type == GITHUB_EASYCONFIGS_REPO:
easyblocks_pr_nrs = [int(pr_nr) for pr_nr in build_option('include_easyblocks_from_pr')]
easyblocks_pr_nrs = [int(eb_pr_nr) for eb_pr_nr in build_option('include_easyblocks_from_pr')]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A safer fix would be using for x in , since we don't use single-letter variables outside of list comprehensions... :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, can we come up with a way to make the tests trip over this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

done in #3726

@lexming lexming deleted the fix-upload-test branch June 5, 2021 14:28
@JensTimmerman
Copy link
Copy Markdown

flake8 should catch this (https://www.flake8rules.com/rules/F812.html)
also, this is fixed in python3:

https://www.python.org/dev/peps/pep-3100/
https://www.python.org/dev/peps/pep-0289/

List comprehensions also "leak" their loop variable into the surrounding scope. This will also change in Python 3.0, so that the semantic definition of a list comprehension in Python 3.0 will be equivalent to list(<generator expression>). Python 2.4 and beyond should issue a deprecation warning if a list comprehension's loop variable has the same name as a variable used in the immediately surrounding scope.
 ~  python3                                                                            Sun 06 Jun 2021 12:48:43 PM CEST
Python 3.9.5 (default, May 14 2021, 00:00:00) 
[GCC 11.1.1 20210428 (Red Hat 11.1.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> i = 6
>>> x = [i for i in [5]]
>>> x
[5]
>>> i
6

perhaps flake8 is ignoring this test because it is not an issue, since flake8 is run in python3 in the tests, I think adding an extra flake8 that runs as python2 would catch these...

@JensTimmerman
Copy link
Copy Markdown

@boegel I've added linting for all supported python versions in #3725
This pr fails because it is now catching errors that were not caught before... so someone might have a look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants