-
Notifications
You must be signed in to change notification settings - Fork 440
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
jhbuild plugin: fix UnboundLocalError for chmod_path #1534
Conversation
Move for-loop into the same scope as the variable `chmod_path`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to have a unit test that covers this bug. We are a now running to get the next release out, and this plugin will be experimental for a few iterations. But do you have in mind how those unit tests would look like? Is that something you can do as a follow up?
Thanks @diddledan!
snapcraft/plugins/jhbuild.py
Outdated
if not os.path.exists(filename): | ||
continue | ||
os.chown(filename, jhbuild_user, jhbuild_group) | ||
for filename in glob.iglob('%s/**' % chmod_path, recursive=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that we are here, could you please use .format
instead of %
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
as requested by code review from @ElOpio
d322a57
to
08c080d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
it is all green according to the link, just not updating here. |
Move for-loop into the same scope as the variable `chmod_path`.
Move for-loop into the same scope as the variable
chmod_path
.fixes: https://bugs.launchpad.net/snapcraft/+bug/1715507
./runtests.sh static
?./runtests.sh unit
?Failing test groups:
None of those touch the jhbuild plugin so I propose the merge anyway.