-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Organization Membership Preview API Changes - existing endpoints #1952
Organization Membership Preview API Changes - existing endpoints #1952
Conversation
fd88e95
to
faa6d0f
Compare
faa6d0f
to
8e8f4af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hnrkndrssn this looks pretty awesome thanks. I noted a couple of things
Octokit.sln.DotSettings
Outdated
@@ -269,6 +269,7 @@ II.2.12 <HandlesEvent />
 | |||
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpKeepExistingMigration/@EntryIndexedValue">True</s:Boolean> | |||
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpPlaceEmbeddedOnSameLineMigration/@EntryIndexedValue">True</s:Boolean> | |||
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpRenamePlacementToArrangementMigration/@EntryIndexedValue">True</s:Boolean> | |||
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpUseContinuousIndentInsideBracesMigration/@EntryIndexedValue">True</s:Boolean> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you clarify what this resharper setting does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...I think this was an involentary change...
/// <summary> | ||
/// Gets or sets the default permission level members have for organization repositories. | ||
/// </summary> | ||
public string DefaultRepositoryPermission { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be an enum?
/// <summary> | ||
/// Default permission level members have for organization repositories. | ||
/// </summary> | ||
public string DefaultRepositoryPermission { get; protected set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be a StringEnum<T>
?
I also noticed while reviewing that there is another API preview involving a new field |
@@ -10,12 +10,13 @@ public class Organization : Account | |||
{ | |||
public Organization() { } | |||
|
|||
public Organization(string avatarUrl, string bio, string blog, int collaborators, string company, DateTimeOffset createdAt, int diskUsage, string email, int followers, int following, bool? hireable, string htmlUrl, int totalPrivateRepos, int id, string nodeId, string location, string login, string name, int ownedPrivateRepos, Plan plan, int privateGists, int publicGists, int publicRepos, string url, string billingAddress, string defaultRepositoryPermission, bool membersCanCreateRepositories) | |||
public Organization(string avatarUrl, string bio, string blog, int collaborators, string company, DateTimeOffset createdAt, int diskUsage, string email, int followers, int following, bool? hireable, string htmlUrl, int totalPrivateRepos, int id, string nodeId, string location, string login, string name, int ownedPrivateRepos, Plan plan, int privateGists, int publicGists, int publicRepos, string url, string billingAddress, string defaultRepositoryPermission, bool membersCanCreateRepositories, string membersAllowedRepositoryCreationType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultRepositoryPermission
and membersAllowedRepositoryCreationType
are string
s in the ctor
rather than the enum types. Although this does compile, we should take the enum type in the ctor for consistency/clarity
👋 Hey Friends, this pull request has been automatically marked as |
Fixes #1514