Skip to content

fix: Address modernize.omitzero issues#3972

Merged
gmlewis merged 2 commits intogoogle:masterfrom
alexandear-org:fix/modernize-omitzero
Feb 6, 2026
Merged

fix: Address modernize.omitzero issues#3972
gmlewis merged 2 commits intogoogle:masterfrom
alexandear-org:fix/modernize-omitzero

Conversation

@alexandear
Copy link
Contributor

@alexandear alexandear commented Feb 5, 2026

This PR enables the previously disabled modernize.omitzero check.

Fixed issues

  1. markReadOptions.LastReadAt:

We can safely change omitzero because it's an unexported struct. Now we can pass an empty Timestamp{} to MarkNotificationsRead and MarkRepositoryNotificationsRead, which means marking all notifications as read.

  1. RepositoryContentResponse.Commit:

It's a required field according to https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28#create-or-update-file-contents.

@alexandear alexandear changed the title fix!: Address 'modernize.omitzero' issues fix!: Address modernize.omitzero issues Feb 5, 2026
@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Feb 5, 2026
@codecov
Copy link

codecov bot commented Feb 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.53%. Comparing base (521b62a) to head (e716b1d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3972   +/-   ##
=======================================
  Coverage   93.53%   93.53%           
=======================================
  Files         207      207           
  Lines       17586    17586           
=======================================
  Hits        16449    16449           
  Misses        937      937           
  Partials      200      200           

☔ 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.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @alexandear!
LGTM.

As far as I can tell, though, this PR does NOT break the existing API, and therefore the PR title can change back to "fix:" instead of "fix!:". Do you agree?

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

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

@alexandear alexandear changed the title fix!: Address modernize.omitzero issues fix: Address modernize.omitzero issues Feb 5, 2026
@stevehipwell
Copy link
Contributor

If you're updating the code for omitzero, the environment created update struct had annotations removed before omitzero was available. The omitzero annotation could now be added to reviewers and one of the other fields (can't remember the name off the top of my head).

Copy link
Contributor

@Not-Dhananjay-Mishra Not-Dhananjay-Mishra left a comment

Choose a reason for hiding this comment

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

LGTM

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 6, 2026

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

@gmlewis gmlewis merged commit c8fd3f7 into google:master Feb 6, 2026
8 checks passed
@alexandear alexandear deleted the fix/modernize-omitzero branch February 6, 2026 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NeedsReview PR is awaiting a review before merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants