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

Add CreateOrUpdateCustomPropertyValues for repositories #3105

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

HariCharan-001
Copy link
Contributor

@HariCharan-001 HariCharan-001 commented Mar 16, 2024

Fixes:
#3101

Followup PR to #3104

Used interface{} type for PropertyValue as it can be string, array or null according to the GitHub documentation here.

In Go, an interface{} can hold any type, including an array (or slice) of strings.

@HariCharan-001 HariCharan-001 changed the title Haricharan2 Add CreateOrUpdateCustomPropertyValues for repositories Mar 16, 2024
Copy link

codecov bot commented Mar 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.88%. Comparing base (2b8c7fa) to head (e581a20).
Report is 43 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3105      +/-   ##
==========================================
- Coverage   97.72%   92.88%   -4.84%     
==========================================
  Files         153      170      +17     
  Lines       13390    11426    -1964     
==========================================
- Hits        13085    10613    -2472     
- Misses        215      723     +508     
  Partials       90       90              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 16, 2024

If you could revert all the changes that are already covered by #3104, that would be great.
Otherwise, we will wait on reviewing this PR until #3104 is merged.

Just a heads-up... we try to avoid using interface{} as much as possible in this repo, but in some places we simply can't avoid it.
I don't have time now to review this PR but will hopefully have time this weekend.

@HariCharan-001
Copy link
Contributor Author

HariCharan-001 commented Mar 16, 2024

The changes in this depend on changes in #3104, so we should wait until it's merged. I don't see otherwise.

" we try to avoid using interface{} "
I see, I'll update if I can do something else instead.

@HariCharan-001
Copy link
Contributor Author

resolved merge conflicts.

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 22, 2024

@HariCharan-001 - as far as I can tell, this PR is not addressing the issue in #3101 which is to add a PATCH endpoint for this: https://docs.github.com/en/rest/repos/custom-properties?apiVersion=2022-11-28#create-or-update-custom-property-values-for-a-repository

@HariCharan-001
Copy link
Contributor Author

@gmlewis
are there any generic issues caused by using interface{} other than lack of type safety and minute impact on performance ?

Do you think it is necessary to avoid using interface{} in this particular scenario ?
I could think of a couple ways, but they may affect code readability and simplicity. What's the trade-off here ?

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 22, 2024

The interface {} issue, I think, is secondary to the issue above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants