Skip to content

Conversation

@Abishek-Newar
Copy link

@Abishek-Newar Abishek-Newar commented Oct 30, 2025

Description

Added a standardized Makefile would make it much easier to test and lint the code locally with simple, consistent commands which would make it easy for contrubuters.

What problem is being solved?

How is it being solved?

What changes are made to solve it?

Added a Makefile to it

References

Closes #138

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

  • Chores
    • Added build automation scripts with commands for testing across multiple .NET framework versions, code formatting validation, and lint checks.

@Abishek-Newar Abishek-Newar requested a review from a team as a code owner October 30, 2025 18:00
@coderabbitai
Copy link

coderabbitai bot commented Oct 30, 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

A new Makefile is added at the repository root with targets for testing across multiple .NET framework versions (net48, net8.0, net9.0), linting, formatting, and a composite check target. The fmt target includes logic to temporarily modify target framework configurations, apply formatting, and restore original project files.

Changes

Cohort / File(s) Summary
New Makefile with CI/testing targets
Makefile
Adds help, test (runs net48/net8/net9 tests), test-net48/test-net8/test-net9 (framework-specific test targets), lint (verifies code formatting), fmt (temporarily modifies project files, applies formatting, and restores originals), and check (runs all checks). Includes platform-aware sed commands for macOS and other Unix variants.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Platform-specific sed syntax for macOS vs. Linux should be verified across common environments
  • Backup/restore logic in the fmt target should be tested for edge cases (e.g., interrupted execution)
  • Verify that hardcoded project file paths match the actual repository structure

Possibly related issues

  • sdk-generator#645: Similar Makefile implementation providing standardized CI targets across SDK repositories to streamline contributor workflows

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning The pull request partially meets the requirements from issue #138, with a notable gap. The Makefile successfully implements the test target with support for all three frameworks (net48, net8.0, net9.0), the lint target for code formatting verification, and the fmt target with the required backup-and-restore mechanism for csproj files. However, according to the provided summary, the check target runs only lint and test, whereas issue #138 explicitly requires check to run all three targets in sequence: fmt, lint, and test. The missing fmt execution in the check target represents incomplete compliance with the stated requirements. Update the check target to execute fmt, lint, and test in sequence as specified in issue #138. The check target should be defined as .PHONY: check with check: fmt lint test to provide the complete workflow validation that contributors expect when running make check.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title accurately and clearly describes the primary change: adding a top-level Makefile to simplify running tests and linters. The title is concise, specific, and avoids vague terminology or unnecessary noise. A developer scanning through commit history would immediately understand the core purpose of this change.
Out of Scope Changes Check ✅ Passed All changes in this pull request are directly aligned with issue #138's requirements. The Makefile includes the four required phony targets (test, lint, fmt, check), implements the specified platform-aware sed commands for multi-target project handling, and includes reasonable enhancements like individual test framework targets (test-net48, test-net8, test-net9) and a help target that are consistent with standard Makefile practices and support the primary objectives.

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.

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: 2

🧹 Nitpick comments (1)
Makefile (1)

47-62: Minor: error handling in fmt target could be more defensive.

The fmt target uses sed with && chaining to ensure both csproj files are modified before proceeding. However, if the sed command fails (e.g., due to an unexpected csproj format), the if statement will exit with an error code, but the Makefile will continue to execute the subsequent dotnet restore and dotnet format commands. While this is unlikely to cause issues in practice, adding explicit error handling (e.g., set -e or explicit checks) would make the target more robust to edge cases.

Consider wrapping the sed commands in a check to exit early on failure:

-	@if [ "$$(uname)" = "Darwin" ]; then \
-		sed -i '' "..." src/OpenFga.Sdk/OpenFga.Sdk.csproj && \
-		sed -i '' "..." src/OpenFga.Sdk.Test/OpenFga.Sdk.Test.csproj; \
-	else \
-		sed -i "..." src/OpenFga.Sdk/OpenFga.Sdk.csproj && \
-		sed -i "..." src/OpenFga.Sdk.Test/OpenFga.Sdk.Test.csproj; \
-	fi
+	@if [ "$$(uname)" = "Darwin" ]; then \
+		sed -i '' "..." src/OpenFga.Sdk/OpenFga.Sdk.csproj && \
+		sed -i '' "..." src/OpenFga.Sdk.Test/OpenFga.Sdk.Test.csproj || exit 1; \
+	else \
+		sed -i "..." src/OpenFga.Sdk/OpenFga.Sdk.csproj && \
+		sed -i "..." src/OpenFga.Sdk.Test/OpenFga.Sdk.Test.csproj || exit 1; \
+	fi

This ensures that if sed fails, the fmt target will halt before attempting to restore or format files.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d596eeb and 3222935.

📒 Files selected for processing (1)
  • Makefile (1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile

[warning] 4-4: Target body for "help" exceeds allowed length of 5 (8).

(maxbodylength)


[warning] 40-40: Target body for "fmt" exceeds allowed length of 5 (22).

(maxbodylength)


[warning] 1-1: Missing required phony target "all"

(minphony)


[warning] 1-1: Missing required phony target "clean"

(minphony)

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.

Add a top-level Makefile in dotnet-sdk to simplify running tests and linters

1 participant