-
Notifications
You must be signed in to change notification settings - Fork 59
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
Repo name/slug/etc. in repository functions #90
Comments
Incidentally, looking back at #73, it seems odd that I need to pass in a slug to get a |
I'm totally agree that methods that take a string parameter named Now how should we rename that parameters and how should we manage it ? I'm less sure to be totally agree with what you exposed. I think that most of the time consumers know the name of their repositories, but less will know the slug, and for the UUID it may even be impossible to know it... Like when we use So, I think it's important to continue to provide an auto 'slugification' in some points of the API (especially public ones). An in that cases I suggest to rename the I'm not in favour of introducing
This doesn't prevents us to support UUID in that parameters, but I think it is preferable to support them like Bitbucket support them, has a UUID string surrounded by curly-braces send in the Our ParseSlug should then looks to something like private string ParseSlug(string repoSlugOrName)
{
if (Guid.TryParseExact(repoSlugOrName, "B", out _))
return repoSlugOrName;
var slugRegex = new Regex(@"[^a-zA-Z\.\-_0-9]+");
return slugRegex.Replace(repoSlugOrName, "-").ToLowerInvariant();
} |
I agree with most of what you say. A few observations:
Should I start a PR so we have something more concrete to discuss? |
For your second point, if we try to support various formats of UUID strings, it may conflict with our unit tests were we create repositories which names are UUIDs without braces (guids formatted with the "N" format, which means 32 digits without braces nor hyphens), which are also valid slug, but definitively not there Uuids Should we reworks our unit tests to avoid to create that case ? @MitjaBezensek can you help us to decide, since we are just two in that discussion with two different point of view ? Except that, you can start a PR since there is probably more discussions to come when we will have the concrete rename locations to decide if such layer should slugify or if it should be done before (I'm not sure that current state is totally the good one without looking at all the cases) |
Good point about the unit tests. My preference would be for the library to accept UUIDs in either D format (hyphens, no braces) or B format (hyphens and braces). This would let users use what are probably the two most common forms and wouldn't affect the unit tests. My thinking is that since the library automatically lowercases when slugifying, so that the user doesn't need to remember that slugs have to be lowercase, it should also add braces if needed so that the user doesn't need to remember that either. I'll start doing some work on a PR. |
The various API calls under
/repositories/{username}/{repo_slug}
can take either the repo slug or the UUID as the final part of the path.The corresponding methods in this library take a string parameter called
repository
, which can be either the slug (abc-repo) or the name (ABC Repo). Before building the URL to call, this parameter is run through a method to slugify it -- to make sure that it looks like a slug by replacing spaces and any other illegal chars with hyphens.This leads to several thoughts:
The parameter is named poorly. It is not clear to a user what a string parameter called
repository
can hold.Users cannot use the UUID to access a repository. BB requires the UUID to be enclosed in
{ }
, andParseSlug()
will turn these characters into hyphens when the URL is constructedIt is not clear why the library allows the user to pass in a repo name, which is silently turned into a slug, when the API doesn't allow a repo name to be used.
I suggest that the parameter should be renamed to
slugOrUuid
, to make it clear what values the user can pass. The library can also provided aSlugify()
helper function in case the user wants to turn a repo name into a slug, but it shouldn't perform this transformation on its own.I'm on the fence as to whether the library should attempt to provide any formatting help if the user passes in a UUID. It would be simple to do:
This would help make sure that the UUID is in the format expected by BB. On the other hand, I can see the argument that this should be the responsibility of the user.
I'm interested in discussion on this topic, and I'm happy to work on a PR if it's decided that some of these changes should be adopted.
The text was updated successfully, but these errors were encountered: