Linkify sha prefixes, display 'repo:' prefix #380
Conversation
| @@ -53,14 +53,16 @@ def disapprove | |||
| end | |||
|
|
|||
| # Attempt to prefix-match a SHA | |||
| def self.prefix_match(git_repo, partial_sha, zero_commits_ok = false) | |||
| def self.prefix_match(git_repo, partial_sha, zero_commits_ok = false, multiple_commits_ok = false) | |||
There was a problem hiding this comment.
allow_ambiguous_match I think is clearer, but in any case, this param could use a description in the method-level comment.
|
Very nice feature Alek. My only concern would be that it results in many more sha lookups, but for small bodies of text that shouldn't matter. As soon as you can take care of a few of the code review comments, I'll merge this in. Looking forward to it! |
| str.gsub(/(^|\s)(([a-zA-Z0-9_-]+):)?([a-zA-Z0-9]{40})/m) do | ||
| repo = Regexp.last_match(3) || repo_name | ||
| sha = Regexp.last_match(4) | ||
| str.gsub(/(^|\s)(([a-zA-Z0-9_-]+):)?([a-zA-Z0-9]{7,40})/m) do |match| |
There was a problem hiding this comment.
The shas coming from git tools are always lowercase, so we could remove A-Z in that character class if we want it to match fewer things. Although the git tooling handles uppercase shas.
There was a problem hiding this comment.
We could do that, but it sounds like a premature optimization, especially since git handles uppercase shas.
|
Fixed in 9bf9e89. |
|
If the sha lookups start to hurt, we can start caching pre-filtered comment text in Redis (although preferably only for older comments, since newer ones sometimes reference commits that haven't been pushed yet), or add an index on the |
|
Nice feature. Just got another request for this today. I'm ready to merge after we confirm my inline comment. |
Apologies if the new testing logic is a bit clumsy; this was the best method I could figure out.