GIT_REMOTE_PATTERNS regexes are being overly greedy #4132
Conversation
With given remote url: “https://github.com/foo/bar.cr.git” - Before: {repo: “bar”} - Now: {repo: “bar.cr”}
|
There has been many changes to those regex. @Sija would you mind adding some specs for these corner cases? I think there are no Doc::Generator specs. If you will, you can put them in This might force some refactor to allow unit testing. It not, let us know and we will pick it from there. |
|
@bcardiff I've added specs according to your suggestion. Any suggestions are welcome :) |
|
Btw, running |
| require "../../../spec_helper" | ||
|
|
||
| GIT_REMOTE_PATTERNS = Crystal::Doc::Generator::GIT_REMOTE_PATTERNS.keys | ||
| private def assert_matches_pattern(url, **options : Object) |
There was a problem hiding this comment.
Remove the : Object restriction and the formatter will success.
Extracted to #4145
| end | ||
|
|
||
| describe Crystal::Doc::Generator do | ||
| describe "GIT_REMOTE_PATTERNS" do |
There was a problem hiding this comment.
Definitely an improvement. Another approach would be to refactor #compute_repository to allow specs for the potential outputs of git remote -v. But I'm ok on keeping it this way for now.
| @@ -0,0 +1,43 @@ | |||
| require "../../../spec_helper" | |||
|
|
|||
| GIT_REMOTE_PATTERNS = Crystal::Doc::Generator::GIT_REMOTE_PATTERNS.keys | |||
There was a problem hiding this comment.
I would avoid declaring this constant. It is used only once.
There was a problem hiding this comment.
Isn't it used every time assert_matches_patten is invoked?
There was a problem hiding this comment.
Ok, so you project the keys. That is not recomputed, but the assert_matches_pattern use .each.compact_map.
I will use Crystal::Doc::Generator::GIT_REMOTE_PATTERNS.each_key.compact_map ... directly.
There was a problem hiding this comment.
np, done as requested.
Sija
force-pushed
the
fix-doc-generator-git-remote-patterns-repo-regex
branch
from
231b9bd
to
194e7c3
Compare
Sija
force-pushed
the
fix-doc-generator-git-remote-patterns-repo-regex
branch
from
194e7c3
to
9cde94d
Compare
|
Thanks @Sija |
This PR fixes overly greedy regexes found in
Crystal::Doc::Generator::GIT_REMOTE_PATTERNS, which atm are stripping last part after (and including) special character given as a part of the repo name.With given remote url: "https://github.com/foo/bar.cr.git" they would incorrectly return
baras arepokey, despite of.crsuffix being legitimate part of the repository name.