Feat: add Triage/Maintain permission by janv8000 · Pull Request #2467 · octokit/octokit.net

janv8000 · 2022-06-28T15:08:42Z

Syncs with current state at https://docs.github.com/en/rest/teams/teams#check-team-permissions-for-a-repository

Should fix #2041

Syncs with current state at https://docs.github.com/en/rest/teams/teams#check-team-permissions-for-a-repository

nickfloyd

Hey @janv8000! Thank you so much for your contributions here! I left a comment on the setter accessors - make the changes if you'd like but it's not a blocker for this changeset.

nickfloyd · 2022-06-29T16:24:52Z

+        /// <summary>
+        /// Whether the current user has maintain permissions
+        /// </summary>
+        public bool Maintain { get; protected set; }


Non-Blocker:
Would you mind changing all of these property accessors (on the set) to the more appropriate (given the use case) private? Using protected is currently the dominant pattern in this SDK but given that most of our models now implement ctors in one form or another so that makes it where I think we could change most of the protected access modifiers to the more appropriate private ones.

Let me know your thoughts - I'll be making a push to change all of the model's properties to use the private modifier soon so I am good with whatever you decide. Once done I'll be adding a theory to our model tests to help enforce this.

Sure thing, I wasn't sure whether the JSON deserializer in use supported private props.
I made them get-only in 40bdd85 or is that pushing the limits :D

✅ I could go for that... given C# 6 and greater these two are essentially equivalent when it comes to constructors but deviate when it comes to methods internal to the model needing to mutate the properties. I really don’t think there will ever be methods that mutate props on response models; that feels like a code smell - but if there is ever a need we can introduce private again.

Also, I'm not sure how getter-only props shake out with model binding (I think that's more of an ASP.NET Core concern), i.e. [FromBody] and the like. So if we ever choose to go in that direction we can reevaluate the pattern.

I think I like your omitted setter idea better than private for future generated response models. 🎉 Thanks for the thought and effort you put in here! We should be good to merge this.

I'm afraid I've been a bit too zealous.
While debugging this branch in my production code the ctor with the 5 arguments is never hit?

Care to shed some light on the SimpleJsonSerializer internals?

Hey @janv8000, so I dug into this a bit more. And it looks like you're correct. We need to at minimum implement a setter with the private modifier.

SimpleJson makes heavy use of reflection to:

Get the constructor and a delegate for the constructor

Iterate through a dictionary (created from the incoming payload) and assign values to each given setter

This is why you only see the default constructor getting hit and not the parameter-based one on incoming payloads.

I am making an assumption here, but I believe that the parameterized constructor is primarily used for testing/mocking - because the delegate uses the memoized object map to make the assignments.

Let me know if that makes sense, and apologies again for missing your question the first time around.

nickfloyd · 2022-06-29T16:28:21Z

-            get { return string.Format(CultureInfo.InvariantCulture, "Admin: {0}, Push: {1}, Pull: {2}", Admin, Push, Pull); }
+            get
+            {
+                return FormattableString.Invariant($"Admin: {Admin}, Maintain: {Maintain}, Push: {Push}, Triage: {Triage}, Pull: {Pull}");


+1 thanks for the simplification here! 🎉

janv8000 · 2022-06-30T14:06:57Z

Your comment on the convention tests got me interested in how they were setup so I started tinkering a bit, see #2471 👨‍🔧

nickfloyd · 2022-06-30T20:24:46Z

@janv8000 I'd like to get this change in for a release tomorrow - are you good with me merging this in or do you want to iterate on it more?

janv8000 · 2022-06-30T21:20:11Z

As I said in #2467 (comment) I’m afraid there is some reflection magic going on somewhere I’m not aware of.
Are you sure get; suffices for the simplejson serializer or does it really need get; private set; ?

nickfloyd · 2022-07-01T01:00:20Z

😬 oof, I completely missed that. So sorry. I'll take a look at this first thing tomorrow.

nickfloyd

Just added comments on the things that need to change to make this work with the serializer. Let me know your thoughts - i'd be glad to do these as well if you're unable to get to it.

Thanks again for the contributions ❇️

nickfloyd · 2022-07-01T20:05:59Z

        /// Whether the current user has administrative permissions
        /// </summary>
-        public bool Admin { get; protected set; }
+        public bool Admin { get; }


Suggested change

public bool Admin { get; }

public bool Admin { get; private set; }

nickfloyd · 2022-07-01T20:06:11Z

+        /// <summary>
+        /// Whether the current user has maintain permissions
+        /// </summary>
+        public bool Maintain { get; }


Suggested change

public bool Maintain { get; }

public bool Maintain { get; private set; }

nickfloyd · 2022-07-01T20:06:23Z

        /// Whether the current user has push permissions
        /// </summary>
-        public bool Push { get; protected set; }
+        public bool Push { get; }


Suggested change

public bool Push { get; }

public bool Push { get; private set; }

nickfloyd · 2022-07-01T20:06:33Z

+        /// <summary>
+        /// Whether the current user has triage permissions
+        /// </summary>
+        public bool Triage { get; }


Suggested change

public bool Triage { get; }

public bool Triage { get; private set; }

nickfloyd · 2022-07-01T20:06:49Z

        /// Whether the current user has pull permissions
        /// </summary>
-        public bool Pull { get; protected set; }
+        public bool Pull { get; }


Suggested change

public bool Pull { get; }

public bool Pull { get; private set; }

See octokit#2467 (comment)

janv8000 · 2022-07-04T09:10:58Z

I made the requested changes + added a test for this convention in 94f2391

terrajobst · 2022-07-11T20:00:43Z

Love it 😍. Thanks 😊

nickfloyd · 2022-07-13T20:24:42Z

release_notes: Adds Triage/Maintain permission properties

feat: add Triage/Maintain permission

3a2be1c

Syncs with current state at https://docs.github.com/en/rest/teams/teams#check-team-permissions-for-a-repository

sbest-gcm approved these changes Jun 28, 2022

View reviewed changes

nickfloyd approved these changes Jun 29, 2022

View reviewed changes

nickfloyd added the category: feature label Jun 29, 2022

feat: made properties strict read-only

40bdd85

nickfloyd mentioned this pull request Jun 30, 2022

Standardize response model property accessors #2472

Closed

nickfloyd suggested changes Jul 1, 2022

View reviewed changes

fix: add private setter for JSON deserialisation

3f5a75f

janv8000 added a commit to janv8000/octokit.net that referenced this pull request Jul 4, 2022

test: add test exposing setterless autoprops

bb87bfc

See octokit#2467 (comment)

test: add test exposing setterless autoprops

94f2391

See octokit#2467 (comment)

janv8000 force-pushed the repository-permissions branch from bb87bfc to 94f2391 Compare July 4, 2022 09:09

janv8000 changed the title ~~[WIP] feat: add Triage/Maintain permission~~ Feat: add Triage/Maintain permission Jul 4, 2022

nickfloyd self-requested a review July 7, 2022 19:32

nickfloyd approved these changes Jul 7, 2022

View reviewed changes

nickfloyd merged commit f92f0b8 into octokit:main Jul 11, 2022

janv8000 mentioned this pull request Jul 16, 2022

[WIP] Model convention tests #2471

Closed

nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022

This was referenced Feb 5, 2026

Bump Octokit from 0.48.0 to 14.0.0 Harshad811/demoproject#108

Open

Bump Octokit from 0.48.0 to 14.0.0 Harshad811/demoproject#114

Open

	public bool Admin { get; }
	public bool Admin { get; private set; }

	public bool Maintain { get; }
	public bool Maintain { get; private set; }

	public bool Push { get; }
	public bool Push { get; private set; }

	public bool Triage { get; }
	public bool Triage { get; private set; }

	public bool Pull { get; }
	public bool Pull { get; private set; }

Conversation

janv8000 commented Jun 28, 2022

Uh oh!

nickfloyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickfloyd Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janv8000 Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janv8000 commented Jun 30, 2022

Uh oh!

nickfloyd commented Jun 30, 2022

Uh oh!

janv8000 commented Jun 30, 2022

Uh oh!

nickfloyd commented Jul 1, 2022

Uh oh!

nickfloyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janv8000 commented Jul 4, 2022

Uh oh!

terrajobst commented Jul 11, 2022

Uh oh!

nickfloyd commented Jul 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nickfloyd Jun 30, 2022 •

edited

Loading

janv8000 Jun 30, 2022 •

edited

Loading