Add claude.md with project conventions and build instructions#93
Add claude.md with project conventions and build instructions#93
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new claude.md document describing project conventions and build/test instructions for the EntityFramework.Exceptions solution.
Changes:
- Introduces
claude.mdwith build/test commands and Docker/Testcontainers requirements - Documents solution structure and architecture (classifiers + interceptors)
- Captures code conventions and provider-specific testing notes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
claude.md
Outdated
| ```bash | ||
| dotnet restore # Restore NuGet dependencies | ||
| dotnet build --no-restore # Build all projects | ||
| dotnet test --no-build # Run tests (requires Docker for Testcontainers) |
There was a problem hiding this comment.
dotnet test --no-build assumes the test projects have already been built. Since the preceding command uses dotnet build (not dotnet build -t:Build on the solution including tests), this can lead to test failures on a clean checkout. Consider changing the test command to dotnet test --no-restore (or drop --no-build) to ensure tests build as needed.
| dotnet test --no-build # Run tests (requires Docker for Testcontainers) | |
| dotnet test --no-restore # Run tests (requires Docker for Testcontainers) |
claude.md
Outdated
| dotnet test --no-build # Run tests (requires Docker for Testcontainers) | ||
| ``` | ||
|
|
||
| Tests use **Testcontainers** and require a running Docker daemon. Each database provider (SQL Server, PostgreSQL, MySQL, Oracle, SQLite) spins up its own container. |
There was a problem hiding this comment.
This sentence states SQLite spins up its own container, but SQLite is typically in-process and does not require Docker. Please correct the wording to avoid misleading setup requirements (e.g., list only providers that use containers and call out SQLite as non-containerized).
| Tests use **Testcontainers** and require a running Docker daemon. Each database provider (SQL Server, PostgreSQL, MySQL, Oracle, SQLite) spins up its own container. | |
| Tests use **Testcontainers** and require a running Docker daemon for containerized providers (SQL Server, PostgreSQL, MySQL, Oracle), each of which spins up its own container. **SQLite** tests run in-process and do not require Docker. |
- Change `dotnet test --no-build` to `dotnet test --no-restore` so tests build as needed on clean checkouts - Clarify that SQLite tests run in-process and don't require Docker, while only containerized providers need a Docker daemon https://claude.ai/code/session_0147Z5VzzZpX1pUfaAapNZie
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add a PostgreSQL-specific test that verifies MaxLengthExceededException is thrown when executing raw SQL that inserts a value exceeding the character varying column limit. This exercises the IDbCommandInterceptor CommandFailed path for PostgreSQL error 22001 (StringDataRightTruncation). The PostgreSQL classifier already handles SqlState 22001 via PostgresErrorCodes.StringDataRightTruncation. This test ensures the full interceptor pipeline works for raw SQL operations in addition to the existing SaveChanges and ExecuteUpdate test coverage. https://claude.ai/code/session_0147Z5VzzZpX1pUfaAapNZie
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ## Code Conventions | ||
|
|
||
| - **C# / .NET 8.0** with file-scoped namespaces, primary constructors, nullable reference types, and implicit usings |
There was a problem hiding this comment.
The statement mentions "implicit usings" as a code convention, but the test project (Tests.csproj) does not have ImplicitUsings enabled. Evidence: explicit using statements are still required in test files (e.g., System.Threading.Tasks was added in PostgreSQLTests.cs), and the Tests.csproj file doesn't contain an ImplicitUsings property. Consider removing "implicit usings" from this list or clarifying that it applies only to certain projects.
| - **C# / .NET 8.0** with file-scoped namespaces, primary constructors, nullable reference types, and implicit usings | |
| - **C# / .NET 8.0** with file-scoped namespaces, primary constructors, nullable reference types; implicit usings are enabled in library projects, while test projects use explicit `using` directives |
|
|
||
| ## Code Conventions | ||
|
|
||
| - **C# / .NET 8.0** with file-scoped namespaces, primary constructors, nullable reference types, and implicit usings |
There was a problem hiding this comment.
The statement mentions "nullable reference types" as a code convention, but none of the csproj files examined (Directory.Build.props, Tests.csproj, PostgreSQL.csproj) contain a Nullable property set to "enable". In .NET 8.0, nullable reference types must be explicitly enabled via the Nullable property in the project file. Consider removing "nullable reference types" from this list or clarifying which projects have this enabled, if any.
| - **C# / .NET 8.0** with file-scoped namespaces, primary constructors, nullable reference types, and implicit usings | |
| - **C# / .NET 8.0** with file-scoped namespaces, primary constructors, and implicit usings |
2 participants
https://claude.ai/code/session_0147Z5VzzZpX1pUfaAapNZie