refactor(cli): introduce cli options including --hierarchical flag
#27
Conversation
|
Blocked by: #28 |
emnul
left a comment
emnul
left a comment
There was a problem hiding this comment.
This structure is definitely much more intuitive and makes sense for more complex applications. It also works fine in the most naive case eg src/doStuff.compact compiles to artifacts/doStuff/**, however that means if a user has the following directory structure:
src
├── doStuff
│ └── otherStuff.compact
└── doStuff.compact
then artifacts will look like:
artifacts/doStuff
├── compiler // doStuff.compact related files
│ └── contract-info.json
├── contract // doStuff.compact related files
│ ├── index.cjs
│ ├── index.cjs.map
│ └── index.d.cts
├── otherStuff
│ ├── compiler
│ │ └── contract-info.json
│ ├── contract
│ │ ├── index.cjs
│ │ ├── index.cjs.map
│ │ └── index.d.cts
│ └── zkir
│ └── otherStuff.zkir
└── zkir
└── doStuffffff.zkir
Beyond being a little confusing I don't think it's worth handling this edge case, but I thought it was worth pointing out.
|
@0xisk what do you think about leaving the flattened structure as the default case and allowing a hierarchical structure via a flag? Flattened works best for the simplest use case, likely the majority of consumers will only have a handful of contracts. Making the default hierarchical seems overly opinionated by preferring the most complex apps. We can have the best of both worlds with a flag 🚩 Creds to @andrew-fleming for the idea. |
0xisk
force-pushed
the
refactor/structured-artifacts
branch
from
f7c4a88 to
39fd5ad
Compare
0xisk
changed the title
--hierarchical flag
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR refactors the compiler's configuration model from positional parameters to an options-driven architecture. It introduces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/cli/src/Compiler.ts (1)
609-646: Argument parsing handles edge cases, but consider positional argument values starting with '+'.The parsing logic correctly handles
--dir,--src,--out,--hierarchical, and version (+<version>) flags. The check!args[i + 1].startsWith('--')prevents treating another flag as a value, but doesn't account for values starting with+(e.g.,--dir +somethingwould be misinterpreted as missing value since+is handled separately).This is a minor edge case unlikely to occur in practice with directory names.
Consider also checking for
+prefix to provide a clearer error message:- const valueExists = - i + 1 < args.length && !args[i + 1].startsWith('--'); + const valueExists = + i + 1 < args.length && + !args[i + 1].startsWith('--') && + !args[i + 1].startsWith('+');README.md (2)
114-118: Add language specifier to fenced code blocks for better rendering.Per markdownlint, fenced code blocks should have a language specified. These blocks show directory structures.
-``` +```text <out>/ ContractA/ ContractB/--- `128-134`: **Add language specifier to this fenced code block as well.** Same issue as above for the hierarchical output example. ```diff -``` +```text <out>/ subdir/ ContractA/ another/ ContractB/</blockquote></details> <details> <summary>packages/cli/src/runCompiler.ts (1)</summary><blockquote> `133-141`: **Error handling for --dir is present, but missing for --src and --out.** The argument parsing error handling only checks for `--dir flag requires a directory name`. Similar errors can be thrown for `--src` and `--out` flags, but they won't trigger the usage help display. Update the error check to handle all directory flag errors: ```diff // Arg parsing const errorMessage = error instanceof Error ? error.message : String(error); - if (errorMessage.includes('--dir flag requires a directory name')) { + if ( + errorMessage.includes('--dir flag requires a directory name') || + errorMessage.includes('--src flag requires a directory path') || + errorMessage.includes('--out flag requires a directory path') + ) { spinner.fail( - chalk.red('[COMPILE] Error: --dir flag requires a directory name'), + chalk.red(`[COMPILE] Error: ${errorMessage}`), ); showUsageHelp(); return; }packages/cli/test/runCompiler.test.ts (1)
215-230: Consider adding tests for --src and --out argument parsing errors.Currently, only the
--dirparsing error is tested. Given the new--srcand--outflags can also throw errors, consider adding test coverage.Add tests for the new flag validation:
it('should handle --src parsing errors', async () => { const error = new Error('--src flag requires a directory path'); mockFromArgs.mockImplementation(() => { throw error; }); await import('../src/runCompiler.js'); expect(mockSpinner.fail).toHaveBeenCalledWith( '[COMPILE] Error: --src flag requires a directory path', ); expect(mockConsoleLog).toHaveBeenCalledWith( '\nUsage: compact-compiler [options]', ); expect(mockExit).toHaveBeenCalledWith(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(2 hunks)packages/cli/src/Compiler.ts(17 hunks)packages/cli/src/runCompiler.ts(3 hunks)packages/cli/test/Compiler.test.ts(18 hunks)packages/cli/test/runCompiler.test.ts(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
114-114: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (15)
packages/cli/src/Compiler.ts (7)
17-20: LGTM! Clear default constants.Good practice defining these as module-level constants for maintainability.
33-70: Well-structured options interface with good documentation.The
CompilerOptionsinterface is well-documented with clear descriptions and examples. TheResolvedCompilerOptionstype correctly combines required defaults with optional fields.
203-237: LGTM! FileDiscovery correctly computes relative paths from srcDir.The refactored
FileDiscoveryproperly usesrelative(this.srcDir, fullPath)to compute paths relative to the source directory, which is essential for the hierarchical output feature to work correctly.
561-577: Well-structured constructor with clear defaults.The constructor properly resolves defaults and wires up the services with the appropriate options. Good separation of concerns.
776-786: Good validation: target directory check only when specified.The condition
this.options.targetDir && !existsSync(searchDir)correctly validates that the target directory exists only when one is specified, avoiding false errors for default full-project compilation.
858-863: Test accessor exposes resolved options.The
testOptionsgetter provides a clean way for tests to verify the internal state without exposing mutable access.
340-349: Edge case for root-level files is correctly handled.The condition
this.options.hierarchical && fileDir !== '.'properly handles root-level files. Verification confirms thatdirname('MyToken.compact')returns'.', causing root-level files to fall back to flattened output regardless of the hierarchical setting, while nested files use hierarchical structure when enabled.README.md (1)
96-162: Comprehensive documentation of CLI options.The new documentation clearly explains all CLI options, artifact structures, and provides useful examples. Good alignment with the implementation.
packages/cli/src/runCompiler.ts (1)
169-236: Help text is comprehensive and well-structured.The updated
showUsageHelpfunction provides clear documentation for all new options with good examples. The artifact output structure explanation is helpful for users.packages/cli/test/runCompiler.test.ts (1)
311-384: Test coverage for usage help is comprehensive.The test properly verifies all new CLI options, the artifact output structure section, and updated examples. Good alignment with the implementation.
packages/cli/test/Compiler.test.ts (5)
300-332: Excellent test coverage for flattened output behavior.These tests properly verify that the default behavior flattens directory structure, handling both single-level and nested paths correctly.
366-418: Good hierarchical output tests with edge case coverage.The tests correctly verify:
- Single-level directory preservation
- Nested directory preservation
- Root-level files remain flattened even with
hierarchical: trueThis edge case (Line 405-417) is particularly important to ensure root-level files don't get incorrect paths.
420-463: Comprehensive tests for custom srcDir and outDir.Tests verify custom source/output directories work correctly, both alone and in combination with hierarchical output. Good coverage.
716-800: Thorough argument parsing tests for new flags.Tests cover:
--hierarchicalflag alone and with other options--srcand--outflags with valid values- Combined
--srcand--outusage- Default values when flags not specified
- Error cases for missing arguments
Excellent coverage of the new CLI options.
585-623: Constructor tests properly updated for options-based API.Tests verify both default values and explicit parameter passing through the new options object pattern.
|
@andrew-fleming @emnul thanks for your suggestions, the recent changes added are for supporting a list of options for the compiler command line including the |
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Great work, @0xisk! I left a comment and a question
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #???
Problem:
The compiler flattened all compiled artifacts into a single
artifacts/directory, losing the source directory structure. This made it hard to navigate artifacts that mirror the source organization (e.g.,src/arccess/,src/math/,src/tokens/, ...).Solution:
Updated
CompilerService.compileFile()to preserve the source directory structure in the artifacts output. The output path now mirrors the source path relative tosrc/.Changes:
dirnameimport to extract directory pathsBefore/After Diff
Artifacts Structure
Before:
After:
This change improves artifact organization and makes it easier to locate compiled contracts that match the source structure.
PR Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.