refactot Route.all_plugins() #1306
Conversation
My code accurately reproduces original functional, but is it correct? In original code `unique` collects plugin names to avoid repeats. If plugin has no name, this check not. So, noname plugin can repeats. Is it right logic?
bottle.py
Outdated
| for plugins in self.plugins, self.app.plugins: | ||
| for p in reversed(plugins): | ||
| if p not in skips and type(p) not in skips: | ||
| name = getattr(p, 'name', False) |
There was a problem hiding this comment.
Do you really need False here ? getattr will return None by default.
There was a problem hiding this comment.
@oz123 getattr() raises AttibuteError by default! So there needs to be a third argument, but IMHO it should be None and not False.
There was a problem hiding this comment.
@oz123, you wrong, if len(args) == 2 and obj has no attr, error rises:
getattr(None, 'name')
Traceback (most recent call last):
File "/usr/lib/python3/dist-packages/IPython/core/interactiveshell.py", line 3331, in run_code
exec(code_obj, self.user_global_ns, self.user_ns)
File "<ipython-input-2-41470b2f07a7>", line 1, in <module>
getattr(None, 'name')
AttributeError: 'NoneType' object has no attribute 'name'
There was a problem hiding this comment.
Yeah, my bad one. Mixed it with dict.get!
bottle.py
Outdated
| if True in self.skiplist: return | ||
| skips = set(self.skiplist) | ||
| for plugins in self.plugins, self.app.plugins: | ||
| for p in reversed(plugins): |
There was a problem hiding this comment.
Compared to the original code I find this much harder to understand.
normally a for loop in python is in the form:
for single in plural
do something ...
You wrote for plugins in <other list of plugins>. I didn't dive into analyzing in depth to see what is the other set of plugins. But this form of for will immediately cause a headache for a lot of people.
Further, an embedded for loop is harder to understand compared to a simple for loop.
|
@SergBobrovsky you do a really cool job on those PRs. Keep up and take the critics as constructive criticism. |
My code accurately reproduces original functional, but is it correct?
In original code
uniquecollects plugin names to avoid repeats. If plugin has no name, this check not. So, noname plugin can repeats. Is it right logic?