fix!: Pass url struct tags by value instead of by reference

[gmlewis]

BREAKING CHANGE: Many *Options structs now pass omitempty URL struct fields by value instead of by reference.

Fixes: #3990.
Relates to: #3984.

Over time, I had allowed and even encouraged pointers to primitive types to be used in the URL struct tags when "omitempty" was provided. This violates the spirit of the purpose of the use of pointers for GitHub resource types as declared in this repo's README.md, and this is the first step to getting back on track to a self-consistent client library.

I apologize for the breakages that this PR causes and for all the contributors whom I wrongly steered during code reviews.

Note that timestamps can still be passed by reference (or by value if using omitzero).

Also note that a new check-structfield-settings tool is added to this repo that will report (and optionally -fix) any obsolete exceptions listed in the .golangci.yml file at the root of the repo.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.52%. Comparing base (e0c53bf) to head (93d9990).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3991   +/-   ##
=======================================
  Coverage   93.52%   93.52%           
=======================================
  Files         207      207           
  Lines       17590    17590           
=======================================
  Hits        16451    16451           
  Misses        938      938           
  Partials      201      201           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

If any of you have time to review, that would be greatly appreciated.

cc: @stevehipwell - @alexandear - @zyfy29 - @Not-Dhananjay-Mishra

[Not-Dhananjay-Mishra]

Also, should we update all the struct in this PR? I believe a lot of struct need to be change

[gmlewis]

Also, should we update all the struct in this PR? I believe a lot of struct need to be change

In this PR, I updated the ones that I know are "safe" - all the pagination ones, etc. I left the others for later review on a one-by-one basis.

[gmlewis]

Thank you, @Not-Dhananjay-Mishra and @alexandear, for the great review feedback!
I believe I've addressed all your comments (except for -fix which I want to keep) and have pushed the changes.
Please take another look when you get a chance.

[gmlewis]

Oh! And here's one other idea I had for the -fix option... we could possibly add it to script/lint.sh and/or script/generate.sh so that it would automatically alert a developer when the ".golintci.yml" file needs updating.
I'm picturing it being used in a non-destructive temporary area like script/generate.sh currently uses so that it could just see if there are any differences caused by running it.

Thoughts?

[alexandear]

Oh! And here's one other idea I had for the -fix option... we could possibly add it to script/lint.sh and/or script/generate.sh so that it would automatically alert a developer when the ".golintci.yml" file needs updating. I'm picturing it being used in a non-destructive temporary area like script/generate.sh currently uses so that it could just see if there are any differences caused by running it.

Great idea. Let's add it to generate.sh.

[gmlewis]

Great idea. Let's add it to generate.sh.

OK, I've incorporated it and pushed the change... amazing that the change was trivial... the comments were more difficult to write. 😂

Here is an example run to show that it is working:

glenn@glenn-MacM2Pro ~/go/src/github.com/google/go-github (i3990-fix-url-tags) $ ./script/generate.sh --check
Obsolete allowed-tag-types:
  - ActivityListStarredOptions.Direction
Generated files are out of date. Please run script/generate.sh and commit the results
diff --git a/.golangci.yml b/.golangci.yml
index 11f70525..51d47ebb 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -231,7 +231,6 @@ linters:
             - WeeklyStats.Deletions
             - WeeklyStats.Week
           allowed-tag-types:
-            - ActivityListStarredOptions.Direction # TODO: Activities
             - APIMetaArtifactAttestations.TrustDomain # TODO: Meta
             - ActionsCacheListOptions.Direction
             - ActionsCacheListOptions.Key
glenn@glenn-MacM2Pro ~/go/src/github.com/google/go-github (i3990-fix-url-tags) $ ./script/generate.sh
Obsolete allowed-tag-types:
  - ActivityListStarredOptions.Direction
glenn@glenn-MacM2Pro ~/go/src/github.com/google/go-github (i3990-fix-url-tags) $ ./script/generate.sh --check
No obsolete structfield exceptions found.

Now I'm wondering if -fix should be silent?
Do you think the message:

$ ./script/generate.sh
Obsolete allowed-tag-types:
  - ActivityListStarredOptions.Direction

might be confusing to end-users?

[Not-Dhananjay-Mishra]

Now I'm wondering if -fix should be silent?

I think we should tell the end users that script has done some changes in .golangci.yml.
How about Removed {x} obsolete fields from .golangci.yml
Thoughts?

[gmlewis]

Now I'm wondering if -fix should be silent?

I think we should tell the end users that script has done some changes in .golangci.yml. How about Removed {x} obsolete fields from .golangci.yml Thoughts?

OK, I thought I saw something wrong, but I think this is what we are ending up with:

glenn@glenn-MacM2Pro ~/go/src/github.com/google/go-github (i3990-fix-url-tags) $ ./script/generate.sh --check
Removed 1 obsolete fields from .golangci.yml.
Generated files are out of date. Please run script/generate.sh and commit the results
diff --git a/.golangci.yml b/.golangci.yml
index f6ac98d3..51d47ebb 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -167,7 +167,6 @@ linters:
         original-url: github.com/google/go-github/v82/tools/structfield
         settings:
           allowed-tag-names:
-            - ActivityListStarredOptions.Direction
             - ActionsCacheUsageList.RepoCacheUsage # TODO: RepoCacheUsages ?
             - AuditEntry.ExternalIdentityNameID
             - AuditEntry.Timestamp
glenn@glenn-MacM2Pro ~/go/src/github.com/google/go-github (i3990-fix-url-tags) $ ./script/generate.sh
Removed 1 obsolete fields from .golangci.yml.
glenn@glenn-MacM2Pro ~/go/src/github.com/google/go-github (i3990-fix-url-tags) $ ./script/generate.sh --check
No obsolete structfield exceptions found.

I saw that "Removed" for the --check but it really is removing it in its own copy... so I think we can't avoid that easily.

[Not-Dhananjay-Mishra]

I saw that "Removed" for the --check but it really is removing it in its own copy... so I think we can't avoid that easily.

If we can't avoid that, I think then it's fine to remove that print statement 🙃

[gmlewis]

I saw that "Removed" for the --check but it really is removing it in its own copy... so I think we can't avoid that easily.

If we can't avoid that, I think then it's fine to remove that print statement 🙃

OK, then -fix will be silent. That's probably better anyway.
Please take another look.

[Not-Dhananjay-Mishra]

LGTM

[gmlewis]

Thank you, @alexandear and @Not-Dhananjay-Mishra!
Merging.