Define the script directory implicitly when DEFINE_LOAD_LIBRARY or INCLUDE_PARSING_CODE needs it - #209
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Resolve custom directory handling and add coverage for implicit parsing-code inclusion.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR makes script-directory initialization implicit for library loading and parsing-code inclusion.
Changes:
- Adds implicit portable directory setup.
- Adds regression fixtures and test registration.
- Updates documentation and changelog.
Outstanding findings:
- Critical: Custom directory handling can omit the required assignment when macros are ordered a certain way.
- Moderate: Coverage is missing for implicit parsing-code inclusion and custom directory names.
File summaries
| File | Description |
|---|---|
tests/regressiontests/test-load-library.m4 |
Adds a library-loading regression template. |
tests/regressiontests/test-library.sh |
Provides the test library fixture. |
tests/regressiontests/Makefile |
Registers the regression test. |
tests/regressiontests/make/tests/tests-base.m4 |
Defines generated test assertions. |
src/stuff.m4 |
Implements implicit directory initialization. |
doc/guide.rst |
Documents the updated macro behavior. |
ChangeLog |
Records the bug fix. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| [m4_ifndef([SCRIPT_DIR_DEFINED], [m4_fatal([You have to define a script directory by some means before using '$0'])])], | ||
| [_ENSURE_SCRIPT_DIR_IS_DEFINED([$2])], | ||
| [m4_list_append([_OTHER], | ||
| m4_expand([[. "$]m4_default_quoted([$2], _SCRIPT_DIR_NAME)[/$1]" [# '.' means 'source' |
There was a problem hiding this comment.
Addressed: INCLUDE_PARSING_CODE now sources from the variable that actually holds the script directory, and if its second argument names a different variable than the one that is already defined (explicitly or implicitly), generation fails with a message that says so. The new gen-test-scriptdir-mismatch covers that ordering.
| argbash_api([INCLUDE_PARSING_CODE], _CHECK_PASSED_ARGS_COUNT(1, 2)[m4_do( | ||
| [[$0($@)]], | ||
| [m4_ifndef([SCRIPT_DIR_DEFINED], [m4_fatal([You have to define a script directory by some means before using '$0'])])], | ||
| [_ENSURE_SCRIPT_DIR_IS_DEFINED([$2])], |
There was a problem hiding this comment.
Addressed: added call-salone-implicit (INCLUDE_PARSING_CODE with a custom variable name and no DEFINE_SCRIPT_DIR) and the gen-test-scriptdir-mismatch generation test.
DEFINE_LOAD_LIBRARY and INCLUDE_PARSING_CODE aborted with an m4 error unless DEFINE_SCRIPT_DIR had been used before them. Both macros now define the script directory themselves (the portable way) if it hasn't been defined yet, so the explicit DEFINE_SCRIPT_DIR is optional. Fixes matejak#148 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DxhC3HmTM7TQUzkBZnyH2
6e0275e to
d2b9109
Compare
Fixes #148
DEFINE_LOAD_LIBRARYandINCLUDE_PARSING_CODEaborted with an m4 error unlessDEFINE_SCRIPT_DIRhad been used before them. Both macros now define the script directory themselves (the portableDEFINE_SCRIPT_DIRway) if it has not been defined yet, so the explicitDEFINE_SCRIPT_DIRline becomes optional.INCLUDE_PARSING_CODEsecond argument (name of the script dir variable) is honored when the directory gets defined implicitly.test-load-library(template withoutDEFINE_SCRIPT_DIRthat loads a library).Tested with
make check(the only failures are the pre-existing shellcheck SC2154 ones in the wrapping tests that #207 addresses).🤖 Generated with Claude Code
https://claude.ai/code/session_017DxhC3HmTM7TQUzkBZnyH2