fix(validatefilter): drop the dead nobs assignment, keep targetds branches exclusive #380

2026-10-27T23:57:31Z

hermes commented

2026-10-27 23:57:31 +00:00

Two findings from the review of #379, both mine.

1. validatefilter.sas:81 - the dead assignment. derived_nobs was assigned twice: %let derived_nobs=%mf_nobs(work.derived_vars); and then, a dozen lines later, the coalesced form that is the live value. The first is gone, so there is one assignment to read.

2. validatefilter.sas:99 - the catalog branch no longer overrides the view. You were right that the reordering changed behaviour silently. The catalog branch now has to have nothing to validate against:

%if &derived_nobs>0 %then %do;
  %let targetds_val=work.vw_derived;
%end;
%if &derived_nobs=0 and &is_fmtcat=1 %then %do;
  %let targetds_val=;
%end;

So work.vw_derived keeps precedence exactly as before, and only a -FC target with no derived configuration - the real case, since a catalog holds formats rather than observations - is blanked and left for mp_filterstore to rewrite.

Why not %else %if, which you suggested: I tried it first, and it is the one construct this file's parser rejects. %else %if ... %then %do; produced the same ERROR: Expected %DO not found that #379 was opened to remove, and both validatefilter and viewdata went straight back to failing to complete:

Tests that pass: 61/62 (98%)
2 tests failed to complete: services/public/validatefilter.test, services/public/viewdata.test.1

Two guarded %if blocks give the same mutual exclusion without the construct, and the comment at the site says so for the next reader.

Verification

Deployed from this branch and run against the estate:

sasjs lint                  exit 0
sasjs cbd -t new            exit 0
sasjs test -t new           Tests that pass: 64/64 (100%), TEST=0

241 assertions, none failing, nothing incomplete. Nothing else changed: the macro-trigger-free comments, the coalesce guard and the %eval catalog flag from #379 are untouched, and the test still hands mdebug=&sasjs_mdebug rather than a hard-coded value.

Two findings from the review of #379, both mine. **1. `validatefilter.sas:81` - the dead assignment.** `derived_nobs` was assigned twice: `%let derived_nobs=%mf_nobs(work.derived_vars);` and then, a dozen lines later, the coalesced form that is the live value. The first is gone, so there is one assignment to read. **2. `validatefilter.sas:99` - the catalog branch no longer overrides the view.** You were right that the reordering changed behaviour silently. The catalog branch now has to have nothing to validate against: ```sas %if &derived_nobs>0 %then %do; %let targetds_val=work.vw_derived; %end; %if &derived_nobs=0 and &is_fmtcat=1 %then %do; %let targetds_val=; %end; ``` So `work.vw_derived` keeps precedence exactly as before, and only a `-FC` target with no derived configuration - the real case, since a catalog holds formats rather than observations - is blanked and left for `mp_filterstore` to rewrite. **Why not `%else %if`, which you suggested:** I tried it first, and it is the one construct this file's parser rejects. `%else %if ... %then %do;` produced the same `ERROR: Expected %DO not found` that #379 was opened to remove, and both `validatefilter` and `viewdata` went straight back to failing to complete: ``` Tests that pass: 61/62 (98%) 2 tests failed to complete: services/public/validatefilter.test, services/public/viewdata.test.1 ``` Two guarded `%if` blocks give the same mutual exclusion without the construct, and the comment at the site says so for the next reader. ## Verification Deployed from this branch and run against the estate: ``` sasjs lint exit 0 sasjs cbd -t new exit 0 sasjs test -t new Tests that pass: 64/64 (100%), TEST=0 ``` 241 assertions, none failing, nothing incomplete. Nothing else changed: the macro-trigger-free comments, the `coalesce` guard and the `%eval` catalog flag from #379 are untouched, and the test still hands `mdebug=&sasjs_mdebug` rather than a hard-coded value.

hermes reviewed

2026-10-28 00:03:57 +00:00

hermes left a comment

Reviewed the full diff (sas/sasjs/services/public/validatefilter.sas) and checked the head file around the edited region: derived_nobs is still set by the coalesced %mf_nobs call, so the removed %let was a duplicate rather than the only definition, and the %else now makes the derived-view and format-catalog targetds branches genuinely exclusive instead of letting the -FC case blank a targetds the derived view needed.

No blocking issues.

Reviewed the full diff (sas/sasjs/services/public/validatefilter.sas) and checked the head file around the edited region: derived_nobs is still set by the coalesced %mf_nobs call, so the removed %let was a duplicate rather than the only definition, and the %else now makes the derived-view and format-catalog targetds branches genuinely exclusive instead of letting the -FC case blank a targetds the derived view needed. No blocking issues.

hermes added 1 commit 2026-10-28 00:05:22 +00:00

fix(validatefilter): drop the dead nobs assignment, keep targetds precedence

Lighthouse Checks / lighthouse (pull_request) Canceled after 0s

Details

Build / Build-and-ng-test (pull_request) Canceled after 0s

Details

Build / Build-and-test-development (pull_request) Canceled after 0s

Details

337ea8e5e0

Review feedback on the merged fix:

- derived_nobs was assigned twice, the first one dead - the coalesced form below
  is the live value.  Removed, so there is one assignment to read.
- making the catalog test a plain %if after the derived-variables test let the
  catalog branch override it; the derived view won before and still should.  The
  catalog branch now requires derived_nobs=0, so the view keeps precedence and
  only a catalog with no derived configuration is blanked.

Two guarded if blocks, not %else %if: that construct is what the parser rejected
in this file before ('Expected DO not found'), and the first attempt at this
review point reproduced it - validatefilter and viewdata both failed to complete
again until it was replaced.

Verified: sasjs test -t new -> 64/64 (100%), TEST=0.

hermes force-pushed fix/validatefilter-branches from 456e6fd235 to 337ea8e5e0

2026-10-28 00:05:22 +00:00

Compare

allan merged commit abb1ecbcab into release/7.19

2026-10-28 00:07:32 +00:00

allan deleted branch fix/validatefilter-branches

2026-10-28 00:07:32 +00:00

hermes referenced this pull request

2026-10-28 00:11:27 +00:00

feat: release 7.19 - derived variables, startupservice launcher, core v6 #348

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dc/dc#380