Conversation
Test Coverage ReportRan Overall coverage: 120/253 macros (47%) PR-changed macrosThis PR is a metadata/packaging change (
No test coverage regression is possible from this PR since no runtime SAS code changed. Generated by Hermes Agent (GitHub App) |
There was a problem hiding this comment.
Hermes Agent Code Review
Verdict: Approve
This is a clean chore(metadata) PR — improved npm packaging, README badges, and a useful SAS skill doc note. No .sas macro files are touched, so there is no runtime/behavioural change and no test-coverage impact. Pre-review checks passed: mergeable: true, mergeable_state: clean, CI (Analyze) success, not a draft.
Critical
None.
Warnings
None.
Suggestions
.npmignore:2—mc_*.sasis correct (it targets the per-folder bundle filesbuild.pyemits:mc_base.sas,mc_ddl.sas, …), but since those files are not committed to git the pattern looks "dead" to anyone reading the ignore list in isolation. A trailing comment line (e.g.# build.py per-folder bundles) would prevent a future maintainer from deleting it. (Inline comment posted.)README.md— the removed[![GitHub closed issues]…]badge was the only badge linking into the issue tracker. Dropping it is a cosmetic call, but if issue-tracker visibility matters to contributors, consider keeping it (or theissuesbadge). Not blocking.
Looks Good
package.json— removing"main": "index.js"is the right call:index.jshas never existed in the repo (confirmed onmain), so the field was pointing npm/Node consumers at a non-existent entry point. For a SAS-macro-only package there is no JS entry point, so omittingmainis correct.package.json— the expandedkeywords(sas-macros,sas9,sas-viya,datastep,etl,sasjs-cli, …) and the more descriptivedescriptionmeaningfully improve npm search discoverability. The prefix hints (mf_*,mp_*,mm*) in the description are accurate (verified against the 408.sasfiles: 88mp_, 51mf_, 45mm)..npmignore—sasjsbuild/andsasjsresults/correctly exclude@sasjs/clibuild output directories from the published tarball..agents/skills/sas/SKILL.md— the new note about read-only automatic macro variables (&syscc,&syswarningtext, …) is accurate and a genuinely useful guardrail (trying%let syswarningtext=;does raiseERROR: Unable to assign value to a macro variable that is read only).skills-lock.json— thesasskillcomputedHashchange is consistent with theSKILL.mdcontent change above; the other skill hashes are untouched.
Test Coverage
Ran npx @sasjs/cli c -t server → 120/253 macros (47%). No macros changed in this PR, so no coverage delta. Full report posted as a separate comment.
Reviewed by Hermes Agent (GitHub App)
Issue
n/a
Intent
Improved packaging
Implementation
Checks
sasjs lint).sasjs test).