Mx createfile - #438
Conversation
Test Coverage ReportOverall coverage: 120/253 macros (47%) Generated by PR-Changed Macros Coverage
Notes
Generated by Hermes Agent (GitHub App) |
![4gl-reviewer[bot]](https://avatars.githubusercontent.com/in/4587573?s=60&v=4){kind=small}
| call symputx('shortloc',shortloc,'l'); | ||
| run; | ||
| data _null_; | ||
| infile &inref lrecl=32767; |
There was a problem hiding this comment.
Suggestion — no input validation for inref=0
The SAS 9 branch does infile &inref lrecl=32767; but the default is inref=0. If the macro is called without specifying inref=, this will resolve to infile 0 which produces a confusing SAS error rather than a clear validation message. The other branches (SASJS, SASVIYA) delegate to ms_createfile/mv_createfile which may handle this, but the SAS 9 branch has open data _null_ steps that will fail opaquely.
Consider adding an early guard at the top of the macro:
%if &inref=0 %then %do;
%put %str(ERR)OR: &sysmacroname requires a valid inref fileref;
%return;
%end;This is also consistent with how mx_execute validates its inputs via %mp_abort.
| data _null_; | ||
| filepath=symget('filepath'); | ||
| name=scan(filepath,-1,'/'); | ||
| shortloc=substr(filepath,1,length(filepath)-length(name)-1); |
There was a problem hiding this comment.
Warning — edge case: filepath with no / separator
name=scan(filepath,-1,'/') and shortloc=substr(filepath,1,length(filepath)-length(name)-1) assume filepath contains at least one /. If a caller passes a bare filename (e.g. settings.sas with no path), shortloc becomes empty or negative-length, which can cause unexpected behaviour in the downstream mv_createfile(path=...) / mm_createstp(tree=...) calls.
The same pattern exists in the SAS 9 branch (line 87) and in the existing mx_getcode.sas macro, so this is a pre-existing convention — but worth noting since mx_createfile is new code. Consider documenting that filepath must be a full path, or adding a guard.
| run; | ||
| data _null_; | ||
| infile &inref lrecl=32767; | ||
| file "%sysfunc(getoption(work))/%superq(name).sas" lrecl=32767; |
There was a problem hiding this comment.
Suggestion — temp file not cleaned up
file "%sysfunc(getoption(work))/%superq(name).sas" lrecl=32767;This writes a physical .sas file to the WORK directory as a temporary step for mm_createstp. The file is never deleted after mm_createstp loads it. Since WORK is cleaned up at session end this is not a leak in practice, but for long-running sessions or mdebug=1 workflows, consider cleaning up:
filename _tmp "%sysfunc(getoption(work))/%superq(name).sas";
%* ... after mm_createstp ...
%if &mdebug=0 %then %do;
data _null_;
rc=fdelete('_tmp');
run;
filename _tmp clear;
%end;
| ,mdebug=&mdebug | ||
| ) | ||
| %end; | ||
| %else %put &sysmacroname: &platform is unsupported!!!; |
There was a problem hiding this comment.
Suggestion — unsupported platform only logs, doesn't set &syscc
%else %put &sysmacroname: &platform is unsupported!!!;This silently continues after printing a message. For comparison, mx_execute uses %put %str(ERR)OR: Unrecognised platform: &platform; (note the ERRORtrick to avoid the SAS ERROR keyword). Consider elevating to an actual error condition by setting&sysccor calling%mp_abortso downstream code doesn't proceed with a false sense of success. This pattern also exists inmx_getcode` so it's a pre-existing convention, but new code could improve on it.
| %inc testref2; | ||
| %end; | ||
| %mend check_content; | ||
| %check_content() |
There was a problem hiding this comment.
Suggestion — check_content macro doesn't handle SASVIYA platform
The check_content macro handles SASJS and SAS9/SASMETA but has no %else %if &platform=SASVIYA branch. For Viya, mx_createfile creates a SAS Content file via mv_createfile, but the test doesn't read it back to verify the content. The assert_result macro does check file existence via %mfv_existfile for Viya, but not content correctness.
This means on Viya, the test only verifies the file exists, not that its content is correct (%let test1=SUCCESS;). Consider adding a Viya branch to check_content that fetches and executes the file content, or document that Viya content verification is deferred.
| %mp_assertscope(COMPARE, | ||
| desc=Test 1: mx_createfile does not leak scope, | ||
| ignorelist=MC0_JADP1LEN MC0_JADP2LEN MC0_JADP3LEN MC0_JADPNUM | ||
| MC0_JADVLEN MC2_JADP1LEN MC2_JADP2LEN MC2_JADPNUM MC2_JADVLEN |
There was a problem hiding this comment.
Looks Good — scope leakage test with ignorelist
Good use of %mp_assertscope(SNAPSHOT) / %mp_assertscope(COMPARE, ...) to catch macro-variable scope leakage, consistent with the testing conventions. The ignorelist for JSON libname engine variables (MC0_JADP*, MC2_JADP*, VIYAPROPERTIES, VIYATYPEDEFNAME) shows attention to platform-specific noise.
There was a problem hiding this comment.
Hermes Agent Code Review
Verdict: Comment — No blocking issues; the PR is well-structured and safe to merge after considering the suggestions below.
Critical
None. The rename is complete with no dangling references to mp_execute or mx_testservice anywhere in the codebase. All dependent macros (mf_getplatform, mm_createstp, ms_createfile, mv_createfile, mf_uid, mfv_existfile) exist and are correctly declared in the doxygen headers.
Warnings
-
inref=0default in SAS 9 branch (xplatform/mx_createfile.sas:92): Thedata _null_; infile &inref ...step will fail with a confusing SAS error ifinrefis left at its default0. The SASJS and SASVIYA branches delegate to macros that may handle this more gracefully, but the SAS 9 branch has open data step code. An early validation guard (%if &inref=0 %then ...) would make the macro more robust. See inline comment. -
filepathwith no/separator (xplatform/mx_createfile.sas:69): Thescan/substrpath-splitting logic assumes at least one/infilepath. A bare filename would produce an empty/negativeshortloc. This is a pre-existing convention (same pattern inmx_getcode.sas) but worth documenting for new code. See inline comment.
Suggestions
-
Temp file cleanup in SAS 9 branch (
xplatform/mx_createfile.sas:93): The physical.sasfile written to WORK formm_createstpis never explicitly deleted. Not a real leak (WORK is session-scoped) but cleaner to remove, especially inmdebug=0mode. -
Unsupported platform handling (
xplatform/mx_createfile.sas:105): The%else %put ... is unsupported!!!branch only logs and does not set&sysccor abort.mx_executeuses the%str(ERR)OR:pattern for the same case. Consider elevating to an error condition so callers are aware. Pre-existing convention inmx_getcode. -
Viya content verification in test (
tests/x-platform/mx_createfile.test.sas:57): Thecheck_contentmacro handles SASJS and SAS9/SASMETA but not SASVIYA. On Viya, only file existence is verified (viamfv_existfile), not content correctness. Adding a Viya branch to read back and verify content would make the test fully cross-platform. -
all.sasmissing trailing newline: The generatedall.sasfile no longer ends with a newline character (confirmed viaod -c). The previous version onmaindid. Since this is a generated file, the CI build should fix this, but it's worth noting.
Looks Good
- Clean rename:
mp_execute→mx_executeis thorough — all references in.sasfiles, test files,all.sas,README.md, andms_testservice.sasare updated. No lingering references to the old names. - Architectural correctness: Moving
mp_executetomx_executecorrectly follows themp_macros must never referencemx_macros convention documented in the updated skill files. The oldmp_executewas a thin wrapper callingmx_testservice, which was an architectural violation. - New
mx_createfilemacro: Well-documented with doxygen header, proper platform dispatching viamf_getplatform(), correct use of%superq()for quoting, and sensible delegation to platform-specific macros (ms_createfile,mv_createfile,mm_createstp). - Test quality:
mx_createfile.test.sascorrectly uses%mp_assertscopefor scope leakage detection, wraps platform-branching code in%macrowrappers (as per the testing conventions), and uses%mf_uid()for unique test artefact naming. - Lint passes:
npx @sasjs/cli lintpasses with no warnings. - Compile passes:
npx @sasjs/cli csucceeds with all PR-changed macros covered by test files.
Reviewed by Hermes Agent (GitHub App)
There was a problem hiding this comment.
Hermes Agent Code Review
Verdict: Comment — The follow-up commit (ee43f9d fix: pr review feedback) cleanly addresses every item from the prior review. The macro and tests are in good shape; one minor suggestion remains below.
Critical
None.
Warnings
None.
Suggestions
frefallocated unconditionally but only used on SAS 9 (xplatform/mx_createfile.sas:62):%let fref=%mf_getuniquefileref();runs at macro entry on every platform, but&frefis only bound to a filename (filename &fref "...") and cleaned up inside the SAS 9 / SASMETA branch. On the SASJS and SASVIYA paths the generated fileref is allocated (viafilename(fname,,temp,...)insidemf_getuniquefileref) and then never used or cleared. Not a real leak (temp filerefs auto-clear at session end), but it's an unnecessary allocation on 2 of 3 platforms. Consider moving the%let fref=%mf_getuniquefileref();(and the%local fref;) into the SAS 9 branch so it is only created when needed. See inline comment.
Looks Good
- Prior feedback fully addressed: The
fix: pr review feedbackcommit adds theinref=0validation guard, documents thatfilepathmust contain a/delimiter, cleans up the SAS 9 temp file viafdelete+filename ... clear(now usingmf_getuniquefilerefper the new AGENTS.md convention), elevates the unsupported-platform branch to%str(ERR)OR:+syscc=1012, and adds a SASVIYA content-verification branch to the test. All six prior review points are resolved. - New AGENTS.md convention: Documenting "Always use
mf_getuniquefilerefwhen assigning filerefs" is a good guardrail and the macro now follows it. - Clean rename:
mp_execute→mx_execute(rename ofmx_testservice) is complete with no dangling references; themp_→mx_boundary convention is respected. - Lint passes:
npx @sasjs/cli lintpasses with no warnings. - Compile + coverage:
npx @sasjs/cli c -t serversucceeds; all PR-changed macros (mx_createfile,mx_execute,mp_ds2csv,ms_testservice) are covered by test files. See the separate Test Coverage Report comment.
Reviewed by Hermes Agent (GitHub App)
Test Coverage ReportRan Overall coverage: 120/253 macros (47%) PR-changed macros
All PR-changed macros that remain in the repo have corresponding test files and are covered. Compile / lint status
Generated by Hermes Agent (GitHub App) |
|
🎉 This PR is included in version 5.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
Hermes Agent Code Review
Verdict: Comment — PR already merged; this reviews the commits pushed after the previous review (9aabeb9, 7e0bba9, b8811db). No blocking issues found.
What changed in the new commits
9aabeb9 fix: pr feedback: Moved%local fref/%let fref=%mf_getuniquefileref()out of the common preamble and into the SAS9 branch only. This avoids allocating an unused fileref on SASJS/Viya. The relocation is correct —%localis resolved at macro compile time, so the late declaration still scopesfreflocally tomx_createfile.7e0bba9,b8811db: auto-regeneratedall.sasand agent skill docs. Verifiedall.sascontainsmx_createfile/mx_executewith no stalemp_execute/mx_testservicereferences anywhere in the tree.
Critical
(none)
Warnings
(none)
Suggestions
- Root-level file edge case in the path-parsing
substr(see inline on line 77). - The path-parsing DATA step is duplicated verbatim between the SASVIYA and SAS9 branches (see inline on line 95).
- The
inref=0early-return guard (xplatform/mx_createfile.sas:64-67) is not exercised by a negative test —mx_createfile.test.sasalways passes a valid fileref. Consider adding a test that asserts the ERROR/early-return wheninrefis omitted. - No validation that
filepathactually contains a/delimiter. A malformed path silently yields emptyshortloc/name, which then get passed downstream tomv_createfile/mm_createstp.
Looks Good
- Rename is complete and consistent across
base/mp_ds2csv.sas,server/ms_testservice.sas, test files, andall.sas. &sysmacronameis used throughoutmx_execute.sas, so internal logging adapts to the new name automatically.- All PR-changed macros (
mx_createfile,mx_execute,mx_getcode) have associated test files and are reported as covered bynpx @sasjs/cli c(see the separate coverage report comment).
Reviewed by Hermes Agent (GitHub App)
| data _null_; | ||
| filepath=symget('filepath'); | ||
| name=scan(filepath,-1,'/'); | ||
| shortloc=substr(filepath,1,length(filepath)-length(name)-1); |
There was a problem hiding this comment.
Edge case — root-level file. For a path like /settings (which satisfies the documented "must contain at least one /" contract), length(filepath)-length(name)-1 evaluates to 0, so shortloc becomes an empty string rather than /.
shortloc=substr(filepath,1,length(filepath)-length(name)-1);On SASVIYA this would then pass path= (empty) to mv_createfile. Consider either tightening the contract to require a parent directory, or special-casing the root, e.g.:
if shortloc='' then shortloc='/';| data _null_; | ||
| filepath=symget('filepath'); | ||
| name=scan(filepath,-1,'/'); | ||
| shortloc=substr(filepath,1,length(filepath)-length(name)-1); |
There was a problem hiding this comment.
Duplicated path-parsing. This DATA step (extracting name and shortloc from filepath) is identical to the one in the SASVIYA branch above (lines 74-80). Consider computing name/shortloc once before the platform if/else so the logic isn't duplicated. (If you'd rather not run it for the SASJS branch where it's unused, a small helper macro would also remove the duplication.)
Test Coverage ReportRan Overall coverage: 120/253 macros (47%) PR-changed macros:
Generated by Hermes Agent (GitHub App) |
1 participant
rename mp_execute to mx_execute (should have been part of the v5 release)
also added a new macro (mx_createfile) and associated test