feat: Convert JD.Efcpt.Build to use JD.MSBuild.Fluent#70
Closed
JerrettDavis wants to merge 79 commits intomainfrom
Closed
feat: Convert JD.Efcpt.Build to use JD.MSBuild.Fluent#70JerrettDavis wants to merge 79 commits intomainfrom
JerrettDavis wants to merge 79 commits intomainfrom
Conversation
…incompatible files. Also updated glob to include all generated subfolders.
…also considered to help avoid path overlap.
…ntainability (#3) * refactor: apply PatternKit patterns to MSBuild tasks for improved maintainability Refactored EnsureDacpacBuilt, ResolveSqlProjAndInputs, and RunEfcpt tasks using declarative PatternKit patterns (Strategy, ResultChain, Composer, Decorator) to improve code readability, maintainability, and testability. Key improvements: - Created 5 shared utilities (CommandNormalizationStrategy, FileResolutionChain, DirectoryResolutionChain, TaskExecutionDecorator, EnumerableExtensions) - Refactored EnsureDacpacBuilt with Strategy patterns for build tool selection and DACPAC staleness detection - Added automatic support for modern Microsoft.Build.Sql SDK projects using 'dotnet build' instead of 'dotnet msbuild' - Refactored ResolveSqlProjAndInputs with Strategy for sqlproj validation, ResultChain for multi-tier file/directory resolution, and Composer for functional state building - Transformed imperative logic into declarative when/then chains across all tasks - Replaced helper methods with functional LINQ pipelines - Introduced immutable record structs for context objects - Eliminated code duplication through shared strategies - Updated `RunEfcpt` Task to utilize `dnx` per (#1) to avoid need for manually install or include Efcpt CLI project or global dependency on .NET10+ * fix: updated builds to include peer dependencies during packaging.
…gineering from a running MSSQL server database (#6)
* docs: add documentation and docfx generation
…nerated files. (#10)
… for data-domain model segregation (#14) * feat: added the ability to split model generation across two projects for data-domain model segregation - Updated README and split-outputs documentation to clarify project roles (#11, #12). - Modified DbContext template to skip foreign keys without navigation properties. - Adjusted project file configurations for Models and Data projects to improve clarity and functionality. - Updated built-in templates to be version aware. - Updated default efcpt-config.json to not exclude all objects (#16)
…tional database providers (#17) * refactor: improve task execution and logging structure - Refactored task execution to utilize a decorator pattern for better exception handling and logging. - Simplified process execution with a new `ProcessRunner` class for consistent logging and error handling. - Enhanced resource resolution chains for files and directories, consolidating logic and improving maintainability. - Updated various tasks to leverage the new logging and execution patterns. * feat(config): add support for MSBuild property overrides in efcpt-config.json * feat(providers): add multi-database provider support for connection string mode Add support for all efcpt-supported database providers in connection string mode: - PostgreSQL (Npgsql) - MySQL/MariaDB (MySqlConnector) - SQLite (Microsoft.Data.Sqlite) - Oracle (Oracle.ManagedDataAccess.Core) - Firebird (FirebirdSql.Data.FirebirdClient) - Snowflake (Snowflake.Data) Key changes: - Create DatabaseProviderFactory for connection/reader creation - Implement provider-specific schema readers using GetSchema() API - Add SQLite sample demonstrating connection string mode - Add comprehensive samples README documenting all usage patterns - Fix MSBuild target condition timing for connection string mode - Add 77 new unit tests for schema reader parsing logic - Update documentation with provider configuration examples * refactor: address PR review comments - Use explicit LINQ filtering instead of foreach with continue - Simplify GetExistingColumn methods using FirstOrDefault - Use pattern matching switch for version parsing logic - Remove unused isPrimaryKey variable in SqliteSchemaReader - Simplify nullable boolean expressions in tests * fix: use string.Equals for fingerprint comparisons in integration tests Replace == and != operators with string.Equals() using StringComparison.Ordinal to address "comparison of identical values" code analysis warnings. * test: add coverage for NullBuildLog, Firebird, and Oracle schema readers - Add NullBuildLog unit tests to cover all IBuildLog methods - Add Testcontainers-based integration tests for FirebirdSchemaReader - Add Testcontainers-based integration tests for OracleSchemaReader - Add Testcontainers.FirebirdSql and Testcontainers.Oracle packages Note: Snowflake integration tests cannot be added as it is a cloud-only service requiring a real account. The existing unit tests cover parsing logic. * docs: add security documentation for SQLite EscapeIdentifier method Address PR review comment by documenting: - Why PRAGMA commands require embedded identifiers (no parameterized query support) - Security context: identifier values come from SQLite's internal metadata - The escaping mechanism protects against special characters in names * fix: pin Testcontainers to 4.4.0 and improve integration test assertions - Downgrade all Testcontainers packages to 4.4.0 for cross-package compatibility (Testcontainers.FirebirdSql 4.4.0 requires matching versions for core library) - Update Firebird and Oracle integration test assertions to use >= 3 instead of == 3 (some database containers may include additional tables beyond the test schema) - Add explicit checks for test tables to ensure schema reader works correctly * feat: add Snowflake integration tests with LocalStack emulator - Add SnowflakeSchemaIntegrationTests using LocalStack Snowflake emulator - Tests skip automatically when LOCALSTACK_AUTH_TOKEN is not set - Add Xunit.SkippableFact package for runtime test skipping - Tests cover schema reading, fingerprinting, and factory patterns Note: LocalStack Snowflake requires a paid token. Tests will run when LOCALSTACK_AUTH_TOKEN environment variable is set, otherwise skip gracefully. * docs: update documentation for multi-database and multi-SDK support - Update samples/README.md to clarify both Microsoft.Build.Sql and MSBuild.Sdk.SqlProj SDKs are supported for DACPAC mode - Fix main README.md: remove outdated "Phase 1 supports SQL Server only" references and update provider support table to show all 7 supported databases (SQL Server, PostgreSQL, MySQL, SQLite, Oracle, Firebird, Snowflake) - Update getting-started.md with multi-database provider examples - Update core-concepts.md with SQL SDK comparison table * refactor: use StringExtensions consistently across schema readers Replace verbose string comparison patterns with extension methods: - `string.Equals(a, b, StringComparison.OrdinalIgnoreCase)` → `a.EqualsIgnoreCase(b)` - `row["col"].ToString() == "YES"` → `row.GetString("col").EqualsIgnoreCase("YES")` Updated files: - SqlServerSchemaReader.cs - PostgreSqlSchemaReader.cs - MySqlSchemaReader.cs - OracleSchemaReader.cs - FirebirdSchemaReader.cs - SnowflakeSchemaReader.cs
…onfig properties, and generated files (#26)
* feat: add JD.Efcpt.Sdk MSBuild SDK package and documentation - Add JD.Efcpt.Sdk as MSBuild SDK for cleaner project integration - Create sdk-zero-config sample demonstrating SDK usage - Extract FileSystemHelpers utility class for code reuse - Add comprehensive SDK integration tests - Update all documentation with SDK approach as primary option - Fix troubleshooting docs for multi-provider support - Clarify CI/CD docs for cross-platform DACPAC builds
* test: add critical regression tests for build package behavior and model generation * feat: add compatibility layer for .NET Framework support This update introduces a compatibility layer for .NET Framework, allowing the project to utilize polyfills for APIs not available in .NET Framework 4.7.2. The changes include conditional compilation directives and the addition of helper methods to ensure compatibility across different target frameworks. * feat: enhance process execution with timeout handling and SQLite initialization - Added timeout handling for process execution to prevent indefinite waits. - Introduced SQLitePCL initialization for Microsoft.Data.Sqlite tests. - Updated project dependencies to include SQLitePCLRaw for SQLite support.
… Full Framework failures (#35)
On .NET Framework MSBuild hosts, certain properties like ProjectDirectory, ProjectReferences, and other MSBuild-set values may be null instead of empty strings. This caused NullReferenceExceptions in: - PathUtils.FullPath when baseDir was null - ResolveSqlProjWithValidation when ProjectReferences or ItemSpec was null - ResourceResolutionChain when searching directories with null paths - ConnectionStringResolutionChain when checking for config files with null directory This fix adds defensive null checks to handle these edge cases, ensuring the build works correctly on both .NET Framework and .NET Core MSBuild hosts.
…n .NET Framework (#38)
…work compatibility (#40)
- Removed /build/ from import paths in CleanTargetTests
- Tests now import from root: {efcptBuildRoot}/JD.Efcpt.Build.props
- Fixes test failures in CI where build/ folder no longer exists
- Changed from MSBuildAssetsOutputPath to direct file references - Files are now generated to project root by fluent - Build folder and buildTransitive subfolder paths corrected
- Added cache clear step before running tests in CI - Ensures tests use freshly built local packages (v1.0.0) - Prevents tests from using old cached v1.0.0 from nuget.org - Fixes 34 SDK integration test failures
- Created coverlet.runsettings with proper XML structure - Fixed coverage collection configuration - Previous inline format with multiple -- was incorrect
…e coverage support
Removed NuGet cache clearing and coverlet.runsettings creation steps. Updated test command to use inline settings for coverage configuration.
Update JD.MSBuild.Fluent package reference from 1.3.4 to 1.3.7 in both JD.Efcpt.Build and JD.Efcpt.Build.Definitions projects to ensure compatibility and leverage latest features.
Package buildTransitive/ files into build/ folder (not buildTransitive/) to prevent transitive propagation. This matches the main branch packaging strategy where source has buildTransitive/ but package only has build/.
There was a problem hiding this comment.
Pull request overview
This pull request converts the JD.Efcpt.Build MSBuild package from hand-written XML to a fluent C# API using the JD.MSBuild.Fluent scaffolder. The PR scaffolds all MSBuild XML files, creates fluent C# definitions, adds a generator console app, and updates test assets and samples accordingly.
Changes:
- Converted MSBuild XML files to fluent C# API definitions using JD.MSBuild.Fluent
- Updated test assets and sample projects to use new import paths (removed
build\subdirectory) - Restructured build and buildTransitive files with new XML formatting and conditional imports
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/JD.Efcpt.Build/JD.Efcpt.Build.props |
New root-level props file with conditional imports |
src/JD.Efcpt.Build/JD.Efcpt.Build.targets |
New root-level targets file with conditional imports |
src/JD.Efcpt.Build/build/* |
Removed old build files, replaced with imports |
src/JD.Efcpt.Build/buildTransitive/* |
Regenerated with JD.MSBuild.Fluent formatting |
src/JD.Efcpt.Build/Definitions/* |
New C# factory classes for MSBuild definitions |
tests/TestAssets/**/*.csproj |
Updated import paths to remove build\ subdirectory |
samples/** |
Added build scripts, NuGet config, and generated SQL files |
*.packages.lock.json |
Added/updated package lock files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The Error task in MSBuild uses Condition for conditional execution, not Code. Updated all Error() calls to pass null for code parameter and condition as 3rd parameter. This fixes the 29 failing tests where !Exists() was being interpreted as an error code instead of a condition.
MSBuild Error task requires Condition attribute, not Code attribute. Fixed all 4 occurrences in buildTransitive/JD.Efcpt.Build.targets. This is the actual file that gets packaged and used by consumers. When fluent generation is working, this will be auto-regenerated from the fixed BuildTransitiveTargetsFactory.cs (commit 6c50899).
- Updated buildTransitive/JD.Efcpt.Build.targets to use Condition= attribute - Updated BuildTransitiveTargetsFactory.cs to pass condition as second parameter - Updated JD.Efcpt.Build.Definitions to use JD.MSBuild.Fluent 1.0.0 with parameter fix - MSBuild Error task requires Condition attribute for conditional errors - Code attribute causes the boolean expression to be treated as literal error code
Version 1.3.9 includes the fixed Error/Warning parameter order.
…overwriting task output
- Add MsBuildNames.cs with well-known MSBuild targets, properties, and tasks - Add EfcptTaskParameters.cs with task-specific parameter names - Add comprehensive documentation in ELIMINATING_MAGIC_STRINGS.md - Implements compile-time safety for MSBuild fluent definitions - Eliminates magic strings and enables IntelliSense/refactoring - Foundation for future source generator implementation - Follows DRY and SOLID principles Benefits: - Compile-time validation prevents typos - IDE support with IntelliSense completion - Safe refactoring with 'Rename Symbol' - Single source of truth for all MSBuild names - XML documentation for discoverability
Critical fixes applied from code review: - Extract ConfigureTaskAssemblyResolution() method * Removes duplication between Props and Targets (16 lines → 1 call) * Single source of truth for MSBuild version detection * Documented resolution strategy and fallback logic - Extract ConfigureNullableReferenceTypes() method * Removes duplication between Props and Targets (4 lines → 1 call) * Documented zero-config derivation from project settings - Create Shared/SharedPropertyGroups.cs * Centralized configuration with comprehensive XML docs * Explains MSBuild version matrix (VS 2017-2026) * Documents fallback strategy for assembly resolution Benefits: - DRY: Eliminated 2 major code duplications - SOLID: Single Responsibility (shared configs in one place) - Maintainability: Changes only need to be made once - Documentation: XML docs explain complex version logic - Cognitive Load: Reduced 40 lines to 2 method calls From CODE_REVIEW.md Phase 1 (Critical Issues #1 & #2)
Comprehensive documentation of: - Phase 1 completion (DRY violations fixed) - Remaining issues identified (Phases 2-5) - Metrics and quality gates - Next steps and priorities Provides roadmap for ongoing refactoring efforts.
These temporary files should never have been committed. Added tmpclaude-* pattern to .gitignore to prevent future accidents. Removed 68 temporary files from tracking.
Phase 2 & 3 improvements from code review: Constants/: - MSBuildVersions.cs - VS version constants - Conditions.cs - Reusable condition expressions with fluent helpers Registry/: - UsingTasksRegistry.cs - Data-driven task registration (16 lines → 5) Updates: - SharedPropertyGroups now uses MSBuildVersions constants - Ready for BuildTransitiveTargetsFactory to adopt these patterns Benefits: - Self-documenting version numbers - Reusable conditions (30+ duplications → helpers) - Data-driven task registration - Easier to maintain and extend
JerrettDavis
force-pushed
the
feature/convert-to-fluent
branch
from
ccdac27 to
d4ea9cb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This demonstrates the scaffolder working on a real-world production MSBuild package!