go-github icon indicating copy to clipboard operation
go-github copied to clipboard

Add CreateOrUpdateCustomPropertyValues for repositories

Open HariCharanK opened this issue 1 year ago • 7 comments

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.

HariCharanK avatar Mar 16 '24 01:03 HariCharanK

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.88%. Comparing base (2b8c7fa) to head (e581a20). Report is 244 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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 16 '24 01:03 codecov[bot]

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.

gmlewis avatar Mar 16 '24 01:03 gmlewis

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.

HariCharanK avatar Mar 16 '24 01:03 HariCharanK

resolved merge conflicts.

HariCharanK avatar Apr 22 '24 19:04 HariCharanK

@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

gmlewis avatar Apr 22 '24 19:04 gmlewis

@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 ?

HariCharanK avatar Apr 22 '24 19:04 HariCharanK

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

gmlewis avatar Apr 22 '24 19:04 gmlewis

I will close this PR around the end of January, 2025 as "abandoned" if there is no further response.

gmlewis avatar Jan 22 '25 01:01 gmlewis

Closing PR as abandoned.

gmlewis avatar Feb 05 '25 14:02 gmlewis