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

Removed overloading of list() #38676

Merged
merged 2 commits into from Jan 15, 2017
Merged

Removed overloading of list() #38676

merged 2 commits into from Jan 15, 2017

Conversation

yhekma
Copy link

@yhekma yhekma commented Jan 11, 2017

What does this PR do?

Fix the consul backend for minion cache

What issues does this PR fix or reference?

#38677

Previous Behavior

When using the consul cache backend, you could run into:
SaltCacheError: There was an error getting the key "set([u'host1', u'host2', u'host3', u'host4'])": unsupported operand type(s) for +: 'set' and 'str' because the function getting the data was called list and also attempted to return a casted list (which turned out to be the function itself)

New Behavior

It works(tm)

Please review Salt's Contributing Guide for best practices.

Yoram Hekma added 2 commits January 11, 2017 08:52
…this function into a recursive one, which results in an exception because set() cannot be concatenated with str ('/')
@DmitryKuzmenko
Copy link
Contributor

@yhekma good catch! Thank you!

@cachedout cachedout merged commit aae8b54 into saltstack:2016.11 Jan 15, 2017
@techhat
Copy link
Contributor

techhat commented Jan 15, 2017

__func_alias__ is the correct way to fix this. I have corrected it in #38660, while rebasing for this PR.

@yhekma
Copy link
Author

yhekma commented Jan 16, 2017

Is it? The reason I did it this way (the renaming of the function) was because I didn't even thing to use __function_alias. Explicit is better than implicit and all that..... (PEP20)

@techhat
Copy link
Contributor

techhat commented Jan 16, 2017

That's why we added __func_alias__. There's examples all over the code if you'd like to check them out.

@yhekma
Copy link
Author

yhekma commented Jan 16, 2017

Yeah, I saw, I guess I just don't like using it because it's cleaner to just use another name imo:)
Regardless, if that's the way it is done in the code, that's the way this fix should be done. Thanks for the fix!

@techhat
Copy link
Contributor

techhat commented Jan 16, 2017

PEP20 is nice and all, but remember that code isn't just for the programmer; it's for the users too. And list is easier to remember and faster to type than getlist. Regardless, I left getlist in there for you too.

@yhekma
Copy link
Author

yhekma commented Jan 16, 2017

For things like id I tend to agree, it's just that I actually use list() once in a while, hence my reluctance. It's easy to remember, but now I also need to remember that in this particular context it means something different than it normally does.
I have to say I didn't check the rest of the code before submitting this PR (which I should have done to ensure consistency) and would have used __func_alias__ otherwise I think.

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