dict update syntax improvement to list_models() #1764
Conversation
This is a code cleanup enhancement originally contributed by @devops117 which didn't get into the 2.2 release. Reissuing the PR now in order to base against main.
| } | ||
| models.update( | ||
| { | ||
| name: { |
There was a problem hiding this comment.
The syntax is a new dict that overwrites the existing keys, so this needs quotes.
| name: { | |
| 'name': { |
There was a problem hiding this comment.
I personally don't understand the original change, or psychedelicious's suggestion.
In my opinion, the original change is switching between equivalent ways of updating a dict (the pre-commit being more readable than the commit).
psychedelicious's suggestion changes the code such that the keys to models are only 'name', and it's constantly overwritten.
Anyway I suggest adding a code comment explaining what is the exact merit of the change, for future reference.
There was a problem hiding this comment.
@psychedelicious is correct; this code will throw an unknown key exception without the quotes.
Having said that, they're "a horse apiece" - using Dict's update() method is more Pythonic, but I find them equally readable, and equally obvious.
There was a problem hiding this comment.
Actually @tildebyte I think @mebelz is correct, name is a variable in this context - the name of the model to be updated. I missed this.
|
Some of the CI tests are stuck and it isn't worth it to unstick them for this bit of syntactic sugar. So closing. |
This is a code cleanup enhancement originally contributed by @devops117 which didn't get into the 2.2 release. Reissuing the PR now in order to base against main.