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

Support Organization/Repository custom properties #2968

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

jackylamhk
Copy link

@jackylamhk jackylamhk commented May 13, 2024

Fixes #2895.

@jackylamhk jackylamhk force-pushed the feat/repo-custom-properties branch from 5cf58d4 to 2f33a50 Compare May 13, 2024 21:49
Copy link
Collaborator

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@hnavarro-kernet hnavarro-kernet left a comment

Choose a reason for hiding this comment

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

Would you mind implementing organization custom properties?
https://docs.github.com/en/rest/orgs/custom-properties

If not don't doubt to tag me in, I'd love this feature as my org recently moved to Github and find this valuable.

Thank you for your work!

"""
url = f"{self.url}/properties/values"
_, data = self._requester.requestJsonAndCheck("GET", url)
return {p["property_name"]: p["value"] for p in data}

Choose a reason for hiding this comment

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

I would return the data directly as expected by the docs

Copy link
Author

Choose a reason for hiding this comment

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

It looks like the repo / and /properties/values return different schemas - I would prefer sticking to dict[str, *] to make it easier to work with. Let me know if you think otherwise :)

Copy link
Author

Choose a reason for hiding this comment

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

One issue I think can of is if GitHub decides to expand the dict with more properties - but otherwise this would make the API easier to work with.

@hnavarro-kernet
Copy link

Closes #2895

@jackylamhk
Copy link
Author

jackylamhk commented May 22, 2024

Would you mind implementing organization custom properties? https://docs.github.com/en/rest/orgs/custom-properties

If not don't doubt to tag me in, I'd love this feature as my org recently moved to Github and find this valuable.

Thank you for your work!

Done - didn't realize it's already returned in the root GET. You're welcome!

@jackylamhk jackylamhk force-pushed the feat/repo-custom-properties branch from 54a16aa to 3bbae39 Compare May 22, 2024 16:45
@jackylamhk jackylamhk changed the title feat: Repository custom properties methods feat: Repository custom properties May 22, 2024
@hnavarro-kernet
Copy link

Would you mind implementing organization custom properties? https://docs.github.com/en/rest/orgs/custom-properties
If not don't doubt to tag me in, I'd love this feature as my org recently moved to Github and find this valuable.
Thank you for your work!

Done - didn't realize it's already returned in the root GET. You're welcome!

There might have been a misunderstanding here, I meant implementing Organization.get_custom_properties, not only Repository,get_custom_properties as mentioned in the docs: https://docs.github.com/en/rest/orgs/custom-properties

@jackylamhk jackylamhk changed the title feat: Repository custom properties feat: Organization/Repository custom properties May 24, 2024
@jackylamhk
Copy link
Author

Would you mind implementing organization custom properties? https://docs.github.com/en/rest/orgs/custom-properties
If not don't doubt to tag me in, I'd love this feature as my org recently moved to Github and find this valuable.
Thank you for your work!

Done - didn't realize it's already returned in the root GET. You're welcome!

There might have been a misunderstanding here, I meant implementing Organization.get_custom_properties, not only Repository,get_custom_properties as mentioned in the docs: https://docs.github.com/en/rest/orgs/custom-properties

My bad, I was looking at the issue you linked earlier. I've written a new OrganizationCustomProperty class and the get methods, let me know if it looks good; I'll finish writing the test cases and update methods.

Considering using the same class for the repo method returns too.

@jackylamhk jackylamhk force-pushed the feat/repo-custom-properties branch from fd2c138 to fc93c6a Compare May 24, 2024 20:20
github/Organization.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

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

Can we rename the get_all_custom_properties method?

Copy link
Collaborator

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

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

Would be great if you could also add the same tests for Organization as you have added for Repository!

@EnricoMi EnricoMi changed the title feat: Organization/Repository custom properties Support Organization/Repository custom properties May 31, 2024
@jackylamhk jackylamhk force-pushed the feat/repo-custom-properties branch 2 times, most recently from 3811b47 to 8a2cf4a Compare June 1, 2024 09:07
@jackylamhk jackylamhk force-pushed the feat/repo-custom-properties branch from 8a2cf4a to 6f424b8 Compare June 1, 2024 09:18
from github.GithubObject import Attribute, NonCompletableGithubObject, NotSet, Opt, is_optional


class CustomProperty:
Copy link
Author

Choose a reason for hiding this comment

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

Should we rename this class to something less confusing (CustomProperty/OrganizationCustomProperty)?

Copy link
Author

Choose a reason for hiding this comment

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

Feel free to make suggestions - can't come up with something simpler.

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.

Support properties for repositories
3 participants