Introduce a type for CodebaseName #996
Conversation
| @@ -2,13 +2,15 @@ package com.sourcegraph.vcs | |||
|
|
|||
| import java.net.URI | |||
|
|
|||
| fun convertGitCloneURLToCodebaseNameOrError(cloneURL: String): String { | |||
| data class CodebaseName(val value: String) | |||
There was a problem hiding this comment.
Is the value always some kind of the url?
If yes, maybe we can make the name more descriptive?
If no, maybe it warrants a comment what value we can expect there?
There was a problem hiding this comment.
I think CodebaseName is the description here
There was a problem hiding this comment.
Codebase name does not suggest url-like-string at all to me.
And it's not defined anywhere what it really means.
Anyway, it's just my point of view, there is nothing wrong with the code per se.
There was a problem hiding this comment.
Codebase name does not suggest url-like-string at all to me.
tbh I'd say that's a good sign - it will be harder to confuse it with URL 😅
| @@ -94,7 +93,9 @@ class AddRepositoryDialog( | |||
| } | |||
|
|
|||
| override fun doOKAction() { | |||
| addAction(repoUrlInputField.text.lowercase()) | |||
| runCatching { convertGitCloneURLToCodebaseNameOrError(repoUrlInputField.text.lowercase()) } | |||
There was a problem hiding this comment.
Can you move that conversion to lowercase inside convertGitCloneURLToCodebaseNameOrError?
That way we will never miss it... as we do now in case of the migration?
| @@ -105,6 +109,20 @@ class SettingsMigration : Activity { | |||
| } | |||
| } | |||
|
|
|||
| private fun migrateUrlsToCodebaseNames(project: Project) { | |||
There was a problem hiding this comment.
Please add a test for that.
Especially considering those cases:
- previously added codebase name without http://
- previously added codebase name with upercase address
| it.isEnabled = true | ||
| it.remoteRepositories = | ||
| mutableListOf( | ||
| RemoteRepositoryState().also { |
There was a problem hiding this comment.
Can you change one of those to show uppercase -> lowercase conversion?
Ideally if that will cause repo to be duplicated it should remove the duplication as well.
There was a problem hiding this comment.
Also you could add different enabled states to prove they are preserved.
mkondratek
force-pushed
the
mkondratek/fix/codebaseName
branch
from
e3d6f24
to
419f13c
Compare
Fixes #967.
Test plan