Skip to content

Address PR #32 review findings - #33

Merged
yudelevi merged 1 commit into
developmentfrom
fix/pr32-review
Sep 24, 2026
Merged

yudelevi merged 1 commit into
developmentfrom
fix/pr32-review

Conversation

@yudelevi

@yudelevi yudelevi commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Fixes the four Greptile findings on #32: changelog conflict marker, bulk companies resume with --exclusion-query-id, file-input OSError/encoding errors escaping the error envelope (also queries save --input), and local geo cross-field validation mirroring the platform's parse_geo_shapes.

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with the targeted fixes matching existing CLI and SDK contracts.

Summary

This PR addresses the prior review findings by:

  • Removing the remaining changelog conflict marker and documenting the corrected behavior.
  • Re-excluding domains from an existing bulk-companies CSV even when separate suppression lists are supplied.
  • Converting file-system and decoding failures into clean CLI parameter errors.
  • Enforcing geo center, radius, and aggregate shape constraints locally in SDK request models.
  • Adding focused regression tests for bulk resume behavior, unreadable inputs, and geo validation.

Reviews (1) · Last reviewed commit: "Address PR #32 review findings before th..."

A leftover conflict marker from the find_emails rebase shipped in the
0.4.0 changelog.

bulk companies skipped re-excluding the resumed CSV whenever any
--exclusion-query-id was passed, assuming those IDs were the run's own
round lists. A customer suppression list is the common case, so the
next page could return only already-written companies and stop early.
The CSV is now always re-excluded.

File inputs caught only FileNotFoundError and JSON errors; a directory,
permission error or non-UTF-8 file escaped as a traceback instead of
the CLI error envelope.

The SDK mirrored the platform's per-shape geo rules but not the
cross-field ones (lat/lon pairing, radius without a centre, invalid
standalone radius, 10-shape total), so those failed only at the API.
@yudelevi
yudelevi merged commit 3759846 into development Sep 24, 2026
8 checks passed
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.

1 participant