Adding regex support for (sub)commands #116
Conversation
david-kn
requested review from
glennmatthews,
jvanderaa and
smk4664
as code owners
| results = [] | ||
| for row in records: | ||
| if row.subcommand == "*": # skip as it is not a valid regex expression and would raise exception | ||
| continue | ||
| if re.match(row.subcommand, value): | ||
| results.append(row.subcommand) | ||
| return results |
There was a problem hiding this comment.
I'm definitely a bit concerned about potential performance impact of this design. It should be possible to do this as an actual DB query, rather than querying for all() and then iterating through the results, though from some quick Googling it looks like custom SQL might be needed to make that possible...
There was a problem hiding this comment.
HI Matthew, thanks for your response and I understand this concern and agree with it.
I'm not familiar with with your code base and didn't want to dig to deep to explore (and reinvent the wheel) the best possible way to interconnect your Django with queries / SQL queries. I rather wanted to kick off this initiative and show that however this might be not that elegant, the update wouldn't have to be extensive to add this neat feature.
Probably some .raw() method to run directly SQL query could help with this (?) as I don't think that django libs would allow such a query. On the other hand, don't you mind writing and mixing SQL queries into your otherwise python code?
On the other hand, amount of these records should be always very limited so the performance should have not been really impacted? But I totally understand and agree that this (performance) aspect as well as conceptual perspective should be always considered and questioned.
I'd leave the final approach to you as your team is in owner/maintainer role :-)
Hi, I just quickly drafted a possible solution for regex matching in (sub)commands which could address issue #115 .
I'm not that familiar with django + their queries and as well this might not be the best possible approach but the logic and sense is incorporated to kick this off ...
Any opinions or comments are welcomed :-)