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

Keep property names that are sub-clients singular #1207

Closed
M-Zuber opened this issue Mar 22, 2016 · 6 comments
Closed

Keep property names that are sub-clients singular #1207

M-Zuber opened this issue Mar 22, 2016 · 6 comments
Labels
Status: Stale Used by stalebot to clean house Type: Feature New feature or request

Comments

@M-Zuber
Copy link
Contributor

M-Zuber commented Mar 22, 2016

The convention is that a client that has sub-clients uses a singular form for the property name
eg

 IObservableTeamsClient Team { get; }

and not

 IObservableTeamsClient Teams { get; }

Is this a check that could be added into the convention tests?
(maybe using something like Humanizer - which might enable us to even just put it in FormatCode and just fix it automagically)

EDIT: we can use this issue to track what places currently need fixing

@shiftkey
Copy link
Member

I'm pretty sure this was something I was tracking as part of auditing everything in #1038 but I'd need to remember just what past me was doing there...

@M-Zuber
Copy link
Contributor Author

M-Zuber commented Mar 22, 2016

Yeah, I've had the email for that issue pinned since it was opened - because some day I will download the code and do a real review 😆

@ryangribble
Copy link
Contributor

I just did a quick search through the code base and found a few properties of clients that are not singular

\Octokit\Clients\IActivitiesClient.cs(14):    IEventsClient Events { get; }
\Octokit\Clients\IActivitiesClient.cs(29):    IFeedsClient Feeds { get; }
\Octokit\Clients\IActivitiesClient.cs(34):    INotificationsClient Notifications { get; }

\Octokit\Clients\IIssuesClient.cs(25):        IIssuesEventsClient Events { get; }
\Octokit\Clients\IIssuesClient.cs(35):        IIssuesLabelsClient Labels { get; }

\Octokit\Clients\IRepositoriesClient.cs(33):  IRepositoryCommentsClient RepositoryComments { get; }
\Octokit\Clients\IRepositoriesClient.cs(49):  IRepositoryDeployKeysClient DeployKeys { get; }
\Octokit\Clients\IRepositoriesClient.cs(215): IRepositoryHooksClient Hooks { get; }
\Octokit\Clients\IRepositoriesClient.cs(221): IRepositoryForksClient Forks { get; }

\Octokit\Clients\IUsersClient.cs(28):         IUserKeysClient Keys { get; }
\Octokit\Clients\IUsersClient.cs(59):         IFollowersClient Followers { get; }

These 2 are also potenitally "awkwardly" named, although the property name does match the API doc, and im not sure if a "better" naming for these "action" based -ing words exists!

\Octokit\Clients\IActivitiesClient.cs(19):    IStarredClient Starring { get; }
\Octokit\Clients\IActivitiesClient.cs(24):    IWatchedClient Watching { get; }

@M-Zuber
Copy link
Contributor Author

M-Zuber commented Mar 22, 2016

These 2 are also potentially "awkwardly" named

IMO The client name is a bit off. If our gospel is the API docs, they should be

IStarringClient
IWatchingClient

Either way Watch and Star can also be adjective-iy so

\Octokit\Clients\IActivitiesClient.cs(19):    IStarredClient Star { get; }
\Octokit\Clients\IActivitiesClient.cs(24):    IWatchedClient Watch { get; }

would not be so jarring ( 😝 ) IMO

In the same vein the StarredRequest class name feels off to me

@devkhan
Copy link
Contributor

devkhan commented Apr 4, 2016

The Migration API has an internal Enterprise Migrations client, so that causes troubles if I follow this. As I can't name both of them Migration, so I have named the internal one as IMigrationsClient Migrations and IMigrationClient. The only difference is the plurality. What should be done in such a case?

Here is the explaination by @ryangribble.

Keeping this aside, I feel that since we follow the GitHub API, should we also make clients nested as in the docs and make the sub-clients as internal namespcaces. This will also solve the problem of Migrations API as we can then have something like Octokit.Migration.IMigration Migration, keeping them singular. Is something like this even possible?

@devkhan devkhan mentioned this issue Apr 6, 2016
12 tasks
@ryangribble ryangribble mentioned this issue Jun 6, 2016
11 tasks
@nickfloyd nickfloyd added Type: Feature New feature or request and removed improvement labels Oct 27, 2022
@github-actions
Copy link

github-actions bot commented Aug 3, 2023

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

@github-actions github-actions bot added the Status: Stale Used by stalebot to clean house label Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale Used by stalebot to clean house Type: Feature New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants