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

win_pkg: Fix traceback when package is not installed #35471

Merged
merged 1 commit into from Aug 16, 2016

Conversation

terminalmage
Copy link
Contributor

@terminalmage terminalmage commented Aug 15, 2016

This corrects two calls to list.pop() which don't account for a potentially-empty list. In the first case, the only way that this happens is if pkg.list_pkgs is broken, so it's very unlikely, but I've added logging there just in case to make an issue with pkg.list_pkgs more apparent.

@terminalmage
Copy link
Contributor Author

I need to take another look at this, I glanced back at the block before this and noticed that the list shouldn't be empty, I'm going to see if there is a better fix.

@terminalmage
Copy link
Contributor Author

OK it should be good now.

@cachedout cachedout merged commit 004778c into saltstack:2015.8 Aug 16, 2016
@damon-atkins
Copy link
Contributor

damon-atkins commented Aug 18, 2016

I thought the standard was to use .format(name) this change seems to remove it from the log messages?

log.trace('Sorting out the latest available version of {0}'.format(name))
vs
log.trace('Determining latest installed version of %s', name)

Could this be updated if log.* does not need to use format
https://docs.saltstack.com/en/latest/topics/development/conventions/style.html

@terminalmage
Copy link
Contributor Author

@damon-atkins I'm not sure what you mean by "standard", to my knowledge there is no standard for log messages.

If you refer to the Python docs, you'll see that printf-style string formatting is already being executed any time a message is logged. The *args are used as input for the % operator under-the-hood. This happens irrespective of whether or not you actually use this syntax. So, using str.format() is (IMO) overkill, since the logging module is already going to do a % string format operation anyway.

@damon-atkins
Copy link
Contributor

There is this statement and then 90% of log messages seem to use format. And the guide says don't use printf which you have indicated is what log does. The aim is to try and remove confusion for my self and I suppose others.

FORMATTING STRINGS
All strings which require formatting should use the .format string method:
data = 'some text'
more = '{0} and then some'.format(data)
Make sure to use indices or identifiers in the format brackets, since empty brackets are not supported by python 2.6.
Please do NOT use printf formatting.

@thatch45
Copy link
Member

Using printf for log messages is ok, it is faster and does not typically require the same level of formatting as inline strings.
I need to update our style guide to reflect this. It was also originally written back in the early days on python3 when the verbiage from python was more around "use tons of .format", but that has changed and they are more open to printf support, (Which has also been expanded and made more robust in recent versions of python3)

@terminalmage terminalmage deleted the issue34479 branch November 30, 2016 03:26
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.

None yet

4 participants