Skip to content

Conversation

@SoulPancake
Copy link
Member

@SoulPancake SoulPancake commented Oct 7, 2025

For #219
Replaced all the type wise ToPtr functions to internally call a common generic function for the same.
Along with deprecation notice of the current ones.

Description

What problem is being solved?

How is it being solved?

What changes are made to solve it?

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

Summary by CodeRabbit

  • New Features
    • Added a generic helper to create pointers for any value type.
  • Deprecations
    • Deprecated legacy type-specific pointer helpers in favor of the new generic approach.
  • Documentation
    • Updated changelog to reflect the new generic helper and deprecations.
  • Tests
    • Added unit tests for the generic pointer helper and to ensure backward compatibility with deprecated helpers.

@SoulPancake SoulPancake requested a review from a team as a code owner October 7, 2025 09:49
@coderabbitai
Copy link

coderabbitai bot commented Oct 7, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Adds a generic ToPtr[T any](v T) *T helper and deprecates existing type-specific PtrX helpers by delegating them to ToPtr. Updates CHANGELOG with the addition and deprecations. Introduces unit tests validating ToPtr and ensuring backward compatibility of PtrX helpers across supported types.

Changes

Cohort / File(s) Summary
Documentation
CHANGELOG.md
Documents new generic ToPtr function and deprecation of PtrX helpers.
Pointer utilities
utils.go
Adds ToPtr[T any](v T) *T. Marks PtrBool/PtrInt/PtrInt32/PtrInt64/PtrFloat32/PtrFloat64/PtrString/PtrTime as deprecated and re-implements them to call ToPtr.
Tests
utils_test.go
Adds unit tests covering ToPtr for multiple types and verifying parity with deprecated PtrX helpers.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes that the PR adds a generic ToPtr function and includes deprecation notices for existing helpers. It is concise and directly related to the main functional change without extraneous details. A teammate scanning the history can understand the primary purpose of the PR from the title.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 34.81%. Comparing base (6863605) to head (a826e0e).

❌ Your project status has failed because the head coverage (34.81%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
+ Coverage   34.75%   34.81%   +0.06%     
==========================================
  Files         111      111              
  Lines       12295    12296       +1     
==========================================
+ Hits         4273     4281       +8     
+ Misses       7670     7663       -7     
  Partials      352      352              

☔ 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

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
utils_test.go (1)

20-212: LGTM with optional refactor suggestion.

The test coverage is comprehensive and correct:

  • TestToPtr validates pointer creation and value correctness for all supported types
  • TestPtrFunctionsBackwardCompatibility ensures deprecated functions still work
  • TestToPtrVsPtrFunctions verifies equivalence between the new and old implementations

Consider using t.Run() to organize test cases for better readability and easier debugging:

 func TestToPtr(t *testing.T) {
-	// Test with bool
-	boolVal := true
-	boolPtr := ToPtr(boolVal)
-	if boolPtr == nil {
-		t.Errorf("ToPtr(bool) returned nil")
-	}
-	if *boolPtr != boolVal {
-		t.Errorf("ToPtr(bool) = %v, want %v", *boolPtr, boolVal)
-	}
+	t.Run("bool", func(t *testing.T) {
+		boolVal := true
+		boolPtr := ToPtr(boolVal)
+		if boolPtr == nil {
+			t.Errorf("ToPtr(bool) returned nil")
+		}
+		if *boolPtr != boolVal {
+			t.Errorf("ToPtr(bool) = %v, want %v", *boolPtr, boolVal)
+		}
+	})
 
-	// Test with int
-	intVal := 42
-	intPtr := ToPtr(intVal)
-	if intPtr == nil {
-		t.Errorf("ToPtr(int) returned nil")
-	}
-	if *intPtr != intVal {
-		t.Errorf("ToPtr(int) = %v, want %v", *intPtr, intVal)
-	}
+	t.Run("int", func(t *testing.T) {
+		intVal := 42
+		intPtr := ToPtr(intVal)
+		if intPtr == nil {
+			t.Errorf("ToPtr(int) returned nil")
+		}
+		if *intPtr != intVal {
+			t.Errorf("ToPtr(int) = %v, want %v", *intPtr, intVal)
+		}
+	})
 
 	// Apply the same pattern to remaining test cases...
 }

This pattern would make test output more granular and help identify specific failures quickly. The same refactor can be applied to the other test functions.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 70e3fe7 and a84ef79.

📒 Files selected for processing (3)
  • CHANGELOG.md (1 hunks)
  • utils.go (1 hunks)
  • utils_test.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
utils_test.go (1)
utils.go (9)
  • ToPtr (22-22)
  • PtrBool (26-26)
  • PtrInt (30-30)
  • PtrInt32 (34-34)
  • PtrInt64 (38-38)
  • PtrFloat32 (42-42)
  • PtrFloat64 (46-46)
  • PtrString (50-50)
  • PtrTime (54-54)
🔇 Additional comments (3)
CHANGELOG.md (1)

5-6: LGTM!

The CHANGELOG entries accurately document the new generic ToPtr function and the deprecation of type-specific pointer helpers. The format is consistent with the existing changelog style.

utils.go (2)

21-22: LGTM!

The generic ToPtr function is simple, idiomatic, and correctly implemented. It eliminates code duplication by providing a single implementation for all types.


24-54: LGTM!

All deprecated pointer helper functions correctly delegate to ToPtr and include proper deprecation notices. This maintains backward compatibility while encouraging users to migrate to the generic implementation.

@SoulPancake SoulPancake force-pushed the ab/use-generics-for-ptr branch from a84ef79 to 9f109e6 Compare October 7, 2025 09:52
@SoulPancake
Copy link
Member Author

Github automatically closed the previous stale PR because of my rebase I guess.
So, I have reopened it here.
@rhamzeh

@SoulPancake
Copy link
Member Author

@coderabbitai generate docstrings

@coderabbitai
Copy link

coderabbitai bot commented Oct 7, 2025

✅ Actions performed

Docstrings generation started.

coderabbitai bot added a commit that referenced this pull request Oct 7, 2025
Docstrings generation was requested by @SoulPancake.

* #244 (comment)

The following files were modified:

* `utils.go`
@coderabbitai
Copy link

coderabbitai bot commented Oct 7, 2025

Note

Generated docstrings for this pull request at #245

@SoulPancake
Copy link
Member Author

I think there are no auxiliary doc changes required for this

sergiught
sergiught previously approved these changes Oct 13, 2025
sergiught
sergiught previously approved these changes Oct 13, 2025
@SoulPancake SoulPancake enabled auto-merge October 13, 2025 11:11
@SoulPancake SoulPancake added this pull request to the merge queue Oct 13, 2025
Merged via the queue into openfga:main with commit b316fa5 Oct 13, 2025
10 checks passed
@SoulPancake SoulPancake deleted the ab/use-generics-for-ptr branch October 13, 2025 11:13
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.

3 participants