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

Rename params to make intent clearer #97

Merged
merged 8 commits into from
Jan 3, 2019
Merged

Rename params to make intent clearer #97

merged 8 commits into from
Jan 3, 2019

Conversation

asherber
Copy link
Contributor

Closes #90

My process here was basically as follows:

  • Any public function that can take a repo slug or name now has a param called repoSlugOrName. It can also take a UUID (mentioned in the XML docs).
  • If a slug or name is passed in, it is converted to a proper slug (character replacement, lowercase) by the XyzResource constructor and stored that way in the resource object.
  • If a UUID in B or D format is passed in, it is converted to B format and stored that way. Any string that happens to be a UUID but is in a different format will not be recognized as a UUID.
  • The private fields on these resource objects are now called _slug, and any time the slug is passed to another function. the parameter is now called slug. Note that through all of this, the use of UUIDs is mentioned only in the XML docs, not in the field or parameter names. This follows the API conventions, as pointed out in Repo name/slug/etc. in repository functions #90 (comment)
  • The same sort of process is followed with anything that can take a team or account name. The parameters and fields still say name, and if a UUID is passed it it is formatted as above and stored in that field.

Note that I have fixed two small bugs in the ParseSlug() (now ToSlug()) method, to make it conform more to what Bitbucket does. Any single quotes in the input string are now removed (instead of converted to hyphens), and any sequence of two or more hyphens is converted to a single hyphen.

Let me know what you think!

Copy link
Collaborator

@mnivet mnivet left a comment

Choose a reason for hiding this comment

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

Small things to change in the StringExtensions class.
But for the main goal it's good for me.


namespace SharpBucket.Utility
{
public static class StringExtensions
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have kept that extension class internal since SharpBucket consumers are not expected to need that helpers.
In particular, as a consumer I don't want my intellisence to be pollute by suggestions of helper methods on the string type that do not concern my domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right, should have been internal. I'll fix it.

if (repoName.TryGetGuid(out var guidString))
return guidString;

if (repoName.Contains('\''))
Copy link
Collaborator

Choose a reason for hiding this comment

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

That test is useless before the call to Replace
The Microsoft documentation clearly state in the returns paragraph that the original string instance is returned by the Replace method if there is nothing to replace, so do not even expect a memory allocation optimisation by doing a call to Contains before doing your call to Replace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, Replace() is an extern call, so I would imagine there's some overhead for calling out to what looks like unmanaged code and therefore a slight benefit to checking Contains() first. But since I'm not calling IsMatch() before the regex Replace(), I may as well take out this check as well.

/// <param name="input">The string to test.</param>
/// <param name="guidString">The Guid in B format, or null.</param>
/// <returns>True if the input string is a Guid in B or D format, or false.</returns>
public static bool TryGetGuid(this string input, out string guidString)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need to expose that method as a public one ?
It's used only as a private one, and even for unit tests it is highly covered by the tests of the two other methods.
So I would be favourable to define it as private.

It's a minor remarks if the class is globally changed to be internal, so if you prefer to keep like that, just tell me.

@mnivet mnivet merged commit 5171641 into MitjaBezensek:master Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repo name/slug/etc. in repository functions
2 participants