Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable all TFS URL's #144

Merged

Conversation

MrHinsh
Copy link
Contributor

@MrHinsh MrHinsh commented Dec 20, 2023

Update both code and tests to enable use with all TFS URL's not just Repositories. This better matches the name and implied function of the tool.

Note: I'm not happy with line 31, and it may need the original coder to analyze intent at this point to validate this works.

 var gitSeparator = new[] { "/_git/" };
 var splitPath = repoUrl.AbsolutePath.Split(gitSeparator, StringSplitOptions.None);
 if (repoUrl.ToString().Contains("_git"))
 {
     this.IsRepository = true;
     if (splitPath.Length < 2)
     {
         throw new UriFormatException("No valid Git repository URL.");
     }

 } else
 {
     this.IsRepository = false;
 }

All tests pass! New tests added for non-repo TFS URL's

…Repositories. This better matches the name and implied function of the tool.

Note: Im not happy with line 31! "if (repoUrl.ToString().Contains("_git"))"
@MrHinsh MrHinsh requested a review from a team as a code owner December 20, 2023 12:08
@christianbumann
Copy link
Member

christianbumann commented Jan 3, 2024

Update both code and tests to enable use with all TFS URL's not just Repositories. This better matches the name and implied function of the tool.

Note: I'm not happy with line 31, and it may need the original coder to analyze intent at this point to validate this works.

 var gitSeparator = new[] { "/_git/" };
 var splitPath = repoUrl.AbsolutePath.Split(gitSeparator, StringSplitOptions.None);
 if (repoUrl.ToString().Contains("_git"))
 {
     this.IsRepository = true;
     if (splitPath.Length < 2)
     {
         throw new UriFormatException("No valid Git repository URL.");
     }

 } else
 {
     this.IsRepository = false;
 }

All tests pass! New tests added for non-repo TFS URL's

Thank you for your Pull Request.
I will review your Pull Request next week.
Do you need a release after merge?

@@ -28,9 +28,17 @@ public RepositoryDescription(Uri repoUrl)

var gitSeparator = new[] { "/_git/" };
var splitPath = repoUrl.AbsolutePath.Split(gitSeparator, StringSplitOptions.None);
if (splitPath.Length < 2)
if (repoUrl.ToString().Contains("_git"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use "/_git/" (like the git separator) otherwise if a repo is named e.g. /abc_git_dev/ this code would not work correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

src/TfsUrlParser/RepositoryDescription.cs Show resolved Hide resolved
@christianbumann
Copy link
Member

@MrHinsh Sorry I've forgotten to submit my review, my mistake

…tion.cs): fix incorrect URL check for Git repository

Correct the URL check to ensure it accurately identifies Git repository URLs. The previous condition was too broad and could lead to false positives. The updated condition checks for the specific "/_git/" segment, ensuring more precise validation.
@MrHinsh
Copy link
Contributor Author

MrHinsh commented Aug 27, 2024

I have updated this for the changes... and the test entry was removed as http://myserver:8080/tfs/defaultcollection/myproject/ is a valid TFS URL.

Copy link
Member

@christianbumann christianbumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for your contribution

@christianbumann christianbumann merged commit bf2d7be into bbtsoftware:develop Sep 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants