Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Make AS's faster #2076
Conversation
erikjohnston
added some commits
Mar 28, 2017
All checks have passed
erikjohnston
assigned
richvdh
Mar 28, 2017
| - if not isinstance(regex_obj.get("regex"), basestring): | ||
| + regex = regex_obj.get("regex") | ||
| + if isinstance(regex, basestring): | ||
| + regex_obj["regex"] = re.compile(regex) |
erikjohnston
Mar 28, 2017
Owner
Err, like this?
regex_obj["regex"] = re.compile(regex) # Pre-compile regexIsn't that commenting on what a line does? I don't see how it helps understanding?
Kegsay
Mar 28, 2017
Contributor
You're clobbering directly over a regex string with a regex object, but fail to say why you're doing this - which is to pre-compile it so you don't need to incur repeated compilation costs. A one-liner inline comment as you have there looks good to me.
erikjohnston
Mar 28, 2017
Owner
Hmm, in which case I think I'll change it to not clobber the existing value, or comment in the function name/docstring.
A comment which is literally describing what a code line is doing always feels wrong to me, and from your comment it sounds like you more dislike the clobbering here than not understanding what's going on. The question is: who is this comment going to help? People who are trying to figure out what the type is will either a) not find the line and guess or b) see from the code its a regex object.
Kegsay
Mar 28, 2017
Contributor
A comment which is literally describing what a code line is doing
It isn't. This is a comment which literally describes what a code line is doing:
# replace regex string with regex object
regex_obj["regex"] = re.compile(regex)What I want to know is why you're doing this. Why don't we keep them as strings and then do it on the fly? Because that's needlessly slow compared to pre-compiling the regex. I don't care about the fact you're clobbering, I care that you fail to describe your intent.
| raise ValueError( | ||
| "Expected string for 'regex' in ns '%s'" % ns | ||
| ) | ||
| return namespaces | ||
| - def _matches_regex(self, test_string, namespace_key, return_obj=False): | ||
| - if not isinstance(test_string, basestring): |
Kegsay
Mar 28, 2017
Contributor
We've lost this type check entirely - is this check expensive enough to warrant that (eg in JS it actually kinda is, introspection of types is painful, python idk).
erikjohnston
Mar 28, 2017
Owner
This is basically against the code style, we never check parameters (except in exceptional circumstances). I think it hurts readability if we do type checking of all parameters, even worse when we only check one parameter.
Besides, the call to re.match will implode if its not a string.
| raise ValueError( | ||
| "Expected string for 'regex' in ns '%s'" % ns | ||
| ) | ||
| return namespaces | ||
| - def _matches_regex(self, test_string, namespace_key, return_obj=False): |
Kegsay
assigned
Kegsay
and unassigned
richvdh
Mar 28, 2017
|
Otherwise LGTM! |
Some checks were not successful
|
Pending comment, but then LGTM |
erikjohnston
merged commit 350333a
into
develop
Mar 31, 2017
0 of 5 checks passed
psaavedra
added a commit
to psaavedra/synapse
that referenced
this pull request
May 19, 2017
erikjohnston commentedMar 28, 2017
No description provided.