Skip to content

docs: clarify accept and reject filtering - #1769

Open
mmaxjr wants to merge 3 commits into
MetPX:developmentfrom
mmaxjr:docs/clarify-filtering
Open

docs: clarify accept and reject filtering#1769
mmaxjr wants to merge 3 commits into
MetPX:developmentfrom
mmaxjr:docs/clarify-filtering

Conversation

@mmaxjr

@mmaxjr mmaxjr commented Aug 28, 2026

Copy link
Copy Markdown

Clarifies how accept/reject filtering is applied across Python flows and C components.

This PR documents that Python flows match against a temporary filtering string, while C components match their component-specific filtered values. It also explains first-match behavior, explicit end-of-pattern matching, and Sundew extension handling as legacy compatibility metadata.

The cleanup/reorganization remains tracked in #1768.

Checked with:

git diff --check
python -m pytest tests/sarracenia/flow/__flow___test.py

@robjarawan robjarawan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a few changes before it merges.

Scope this to the component doing the filtering

The options page is shared by both implementations, so saying the patterns use Python re.match isn’t true for cpost and cpump. The Python flows compile with re and call Pattern.match(). The C components use POSIX regexec() instead; cpost filters the pathname, cpump filters relPath, and neither appends the separate sundew_extension header.

Could we document those separately so the leading .* advice isn’t applied to the C components?

Describe the filtering string, not a changed URL

For Python flows, the code builds a temporary urlToMatch from baseUrl + relPath. It conditionally appends :<sundew_extension> when the header exists and the URL has fewer than three colons. It doesn’t modify the notification URL itself.

Can we describe this as the string used for accept/reject filtering and avoid saying the extension is always included?

Keep the published copies in sync

This currently updates only docs/source/Reference/sr3_options.7.rst. The same FIXME and filtering guidance also appear in the English command-line guide and both French copies.

Could we update all four sources here? Otherwise we’ll publish conflicting explanations depending on which page someone reads.

One other contradiction: the new note correctly says .*\.gif also matches image.gif2, but the following paragraph still says the rule rejects files “ending in gif.” I’d also use .*\.csv and .*\.csv$ consistently in the examples and state directly that the first matching accept/reject rule wins.

I ran git diff --check, the focused filtering tests, and the documentation build. The changed page renders correctly.

@robjarawan

Copy link
Copy Markdown
Contributor

Small correction to my earlier example after Peter’s note on #1768: .*\.csv is too loose because it also matches something like .csvhithere.txt.

If the filter is specifically for messages carrying a Sundew extension, .*\.csv:.* makes the colon boundary explicit. If it needs to accept both a plain .csv filename and .csv:<sundew_extension>, we can use .*\.csv$|.*\.csv:.

I’d also describe sundew_extension as legacy compatibility metadata and recommend avoiding it in filters when the path alone is enough. The Python/C and temporary match-string distinctions from my earlier review still apply.

@mmaxjr
mmaxjr marked this pull request as ready for review August 29, 2026 00:09
@mmaxjr

mmaxjr commented Aug 29, 2026

Copy link
Copy Markdown
Author

Updated the PR to address the review feedback:

  • scoped the regex wording so Python flows and C components are documented separately
  • described the temporary Python filtering string instead of saying the notification URL is modified
  • updated the English reference page, English command-line guide, and both French copies
  • replaced the .*\.gif wording that implied an end-of-string match
  • changed the CSV examples to use explicit .csv, .csv:, and .csv$ boundaries
  • noted that the first matching accept or reject rule wins

Validation:

  • git diff --check
  • python -m pytest tests/sarracenia/flow/__flow___test.py

I also tried a local Sphinx build. It starts, but this local environment is missing pandoc, so it stops while processing Contribution/Philosophy/AboutTime.ipynb before reaching these changed pages.

@robjarawan

Copy link
Copy Markdown
Contributor

Thanks, I went through the updated diff. The Python/C split, temporary filtering string, first-match behaviour, and four published copies are all covered now. The focused filtering tests pass and the docs build completes here.

I found one remaining consistency pass before I approve. The new text correctly explains that the patterns operate on a component-specific filtering value, but the same pages still say accept and reject match the URL in a few places. The command-line guides also still say .*cap means “ends with CAP”; that example needs .*cap$, since the new note correctly explains that there’s no implicit end-of-string match.

Could we replace the remaining URL references with “filtering string” or “filtered value” as appropriate, and change that ending example to .*cap$ in both the English and French guides? Then I think this is ready.

@mmaxjr

mmaxjr commented Aug 31, 2026

Copy link
Copy Markdown
Author

Updated the consistency pass:

  • replaced the remaining URL-based accept/reject wording in the edited sections with filtered value/filtering string wording
  • changed the CAP ending examples to use .*cap$ / .*CAP$
  • kept the changes in sync across the English reference, English guide, French reference, and French guide

Checked with:

git diff --check
python -m pytest tests/sarracenia/flow/__flow___test.py

@reidsunderland

Copy link
Copy Markdown
Member

The changes so far also look good to me. There is duplication between the command line guide and sr3 options documents.

The other part of #1768 is to clean up the documentation and remove duplication. There's a lot of overlap between the command line guide and sr3 options related to filtering. I think we need to resolve that, but this PR can focus on clarification and we can create another one to work on cleanup/reorgination.

@robjarawan robjarawan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now. I checked 6b48694; the four copies are in sync, the focused tests pass, git diff --check is clean, and the docs build completes here.

I agree with keeping the exhaustive details in sr3_options and handling the command-line guide duplication separately.

One housekeeping item before merge: the PR description still mentions only re.match and says Sundew extensions are included in filtered URLs. It also closes #1768, but I don’t see a separate issue tracking the cleanup yet. Could we update the summary and either create/link the cleanup issue or leave #1768 open?

The existing .*?cap wording can go into that cleanup pass. It isn’t a blocker for this PR.

@mmaxjr

mmaxjr commented Aug 31, 2026

Copy link
Copy Markdown
Author

Updated the PR description to reflect the current scope and avoid closing #1768.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants