octokit.net
octokit.net copied to clipboard
Keep property names that are sub-clients singular
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
I'm pretty sure this was something I was tracking as part of auditing everything in https://github.com/octokit/octokit.net/pull/1038/ but I'd need to remember just what past me was doing there...
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 :laughing:
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; }
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 ( :stuck_out_tongue_closed_eyes: ) IMO
In the same vein the StarredRequest
class name feels off to me
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?
👋 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!