code review.

This commit is contained in:
Nathan TeBlunthuis 2023-05-03 10:23:30 -07:00
parent 54fa6221a8
commit 933ca753ed
3 changed files with 22 additions and 16 deletions

View File

@ -16,4 +16,4 @@ files. Therefore wikiq depends on `7za`, `gzcat`, and `zcat`.
There are also a series of Python dependencies. You can install these using pip There are also a series of Python dependencies. You can install these using pip
with a command like: with a command like:
pip3 install mwbase mwreverts mwxml mwtypes mwcli mwdiffs mwpersistence pip3 install mwbase mwreverts mwxml mwtypes mwcli mwdiffs mwpersistence pandas

9
code_review_notes.txt Normal file
View File

@ -0,0 +1,9 @@
Please add unit tests for the new count-only functionality.
line 43 def matchmake:
This was making redundant calls to regex matching functions and so could be slower than necessary. I suggest changes that use the walrus operator to keep the same logical structure without the redundant calls.
line 212 def __init__:
Minor note: This constructor is taking a lot of arguments. This is fine, but from a style + maintainability perspective it might make sense to create a new class for the regex matching configuration and pass a configuration object to this contructor instead.

9
wikiq
View File

@ -145,17 +145,14 @@ class RegexPair(object):
if self.has_groups: if self.has_groups:
# if there are matches of some sort in this revision content, fill the lists for each cap_group # if there are matches of some sort in this revision content, fill the lists for each cap_group
if content is not None and self.pattern.search(content) is not None: if content is not None and len(matchobjects := list(self.pattern.finditer(content))) > 0:
m = self.pattern.finditer(content)
matchobjects = list(m)
for cap_group in self.capture_groups: for cap_group in self.capture_groups:
key = self._make_key(cap_group) key = self._make_key(cap_group)
temp_list = [] temp_list = []
for match in matchobjects: for match in matchobjects:
# we only want to add the match for the capture group if the match is not None # we only want to add the match for the capture group if the match is not None
if match.group(cap_group) != None: if (group := match.group(cap_group)) is not None:
temp_list.append(match.group(cap_group)) temp_list.append(group)
# if temp_list of matches is empty just make that column None # if temp_list of matches is empty just make that column None
if len(temp_list)==0: if len(temp_list)==0: