Add plan for 3.7.0 to WAL - #314
Open
AlexeyTrekin wants to merge 19 commits into
Open
Conversation
Replaces the ported generic scaffold with a version grounded in this repo.
The port arrived carrying a backend service's assumptions -- /app + alembic +
docker-compose, uvicorn reload, psql diagnostics, master as the integration
branch -- none of which exist here. An agent following it blocked on the
first build command.
## Structure
- .github/instructions/*.instructions.md -> instructions/*.md
- ui.delivery.instructions.md -> instructions/lang/ui.md, scoped mapflow/dialogs/**
- instructions/lang/python.md rescoped to {mapflow,tests}/** with PyQGIS conventions
replacing the alembic/SQLAlchemy/Pydantic sections
## Subagent fixes
- Dropped dangling references to AGENTS.md sections that do not exist
(ENFORCED COMMAND BOUNDARY, PHASE DELEGATION TIERS) plus the tier language
that fed off them.
- Replaced nonexistent make targets (build, unittest, test-attach, logs, ps,
psql-diag) with the real ones, and removed the uvicorn reload rule -- the
repo is bind-mounted into the test container, so edits need no reload.
- Write scopes app/** -> mapflow/**, docker-compose.yaml -> Dockerfile.tests.
- origin/main -> origin/master in the stabilizer stop condition.
- Reviewer diffs dev...HEAD and no longer claims agent-make diagnostics.
## Branch model
dev stays the integration branch, but master is what agent-make verifies
watched files against. Documented explicitly so a Makefile/Dockerfile.tests
change gets planned as its own MR: merging it to dev alone leaves agent-make
blocked for every later step. Relevant immediately, since WAL 3.7.0 step 1
changes the lint target for flake8/bandit/detect-secrets.
## Test plan
- No source or test files touched; this MR cannot affect agent-make results.
- Four subagents load cleanly in Claude Code.
Finishes WAL 3.7.0 section 1's linter bullet: removes the ruff/pyright remnants, configures flake8, and makes lint a blocking CI job. ## Why the gate goes in before the cleanups CI ran no lint at all. Without a ratchet, every finding-reduction MR is a snapshot the next MR silently undoes -- which is how ~1425 findings accumulated. The gate lands first; the cleanup bullets land behind it. ## Findings: ~1425 -> ~811 Closed outright: - W191 x539 (38% of all findings) -- one test file, uniform tab indentation, converted to 4 spaces. is empty, proving no code changed. - F403/F405 -- one star-import replaced with an explicit import. - F401 73 -> 1 via per-file-ignores for __init__.py, restoring the ignore ruff carried before the migration. Deferred into a documented .flake8 ledger, each entry naming the step that removes it: E722 (bullet 1), F401 residual (circular imports that survive only via partial-module caching -- a nominally unused import may be load-bearing), and the whitespace classes. Whitespace normalisation waits until immediately before the refactor branch cut: it concentrates in mapflow.py and the service layer, which the refactoring rewrites, and would make the in-flight feature/track-uploaded-image-status rebase worse. ## pyright is dropped, and that is a real loss Verified against the qgis.org scanner: it runs flake8 + bandit + detect-secrets, and pyright is not among them. Nothing in the new toolchain replaces reportPossiblyUnbound -- flake8's F821 covers undefined names only -- in a codebase with broad except handlers that would mask exactly that. Accepted for qgis.org parity, recorded in WAL, revisit after annotations land. ## Spec correction 004_stack.md's dependency policy forbade non-QGIS-bundled libraries outright, which was already false on master: Dockerfile.tests installs pytest and three linters. Scoped to runtime plugin deps, with the evidence -- .qgis-plugin-ci packages the mapflow/ directory alone, so dev tooling never ships. ## nosec syntax The comments emitted a bandit warning per site per run. An ID prefix alone does not fix it: bandit captures everything after up to the next , so trailing prose keeps being parsed as test IDs. Correct form is . Warnings now zero. No suppression changed scope. ## Verification - agent-make lint: exit 0, all three checks clean - agent-make test: 8 functional, 403 qgis, UI harness green - Known caveat: the tab conversion also normalised ~6 docstring-internal indentation lines. git diff -w cannot see inside string literals. Harmless (nothing asserts on docstring text) but not covered by that evidence. - Touches no watched file: Makefile and Dockerfile.tests are unchanged.
Removes the two asserts in ErrorMessageList.update() that bandit flagged as B101, and enforces the same invariant from tests instead. This is WAL 3.7.0 section 1, bullet 2 -- pulled forward because these were the last findings blocking a green lint run after the bandit severity floor was removed. ## Why a test rather than a runtime check errors.py merges ProcessingErrors, DataErrors and ApiErrors at import, and update() delegates to dict.update, which overwrites silently. A duplicated code would shadow one description and show the user the wrong error text with no signal. Those three registries are statically defined, so the invariant is a property of the source, not of any given run. An assert is the wrong tool twice over: it is stripped under python -O -- precisely the packaged builds where a silent wrong message is hardest to notice -- and it costs work at every plugin start-up to check something that cannot vary at runtime. ## Tier: functional, not qgis The WAL bullet specified tests/qgis/. The check is pure dict-key logic touching no QGIS state, so spec/004_stack.md's tier definition places it in tests/functional/, and AGENTS.md SPECIFICATION GUIDELINES rule 2 makes the spec win over the WAL line. tests/functional/test_error_message.py already imports the same module. WAL updated to record the correction. ## Test shape Pairwise across the three registries rather than a single merged-length assertion, so a failure names the offending pair and the colliding key rather than reporting an unhelpful count mismatch. A merged-registry check backs it up to catch entry loss for any other reason. Verified by mutation: planting a duplicate key makes exactly the relevant pair fail while the other pairs keep passing. A collision test that cannot fail would have been worse than no test. ## Verification - agent-make lint: exit 0, all three checks clean, zero bandit warnings - agent-make test: 15 functional (7 new), 403 qgis, UI harness green
WAL.md was defined as a permanent journal of completed steps. It is now a plan + in-flight tracker holding only planned and in-flight entries; a step's entry is removed when it merges. ## Why The WHY of a completed change has two better homes. The commit message keeps it attached to the diff that made it, and spec/ keeps durable decisions findable without knowing which commit to look at. A journal entry is a third copy that drifts from both and grows without bound -- the two entries removed here had reached 15 and 27 lines against an instruction that asked for a concise motivation, which is what prompted this. ## Changes - PROJECT STRUCTURE: new WAL definition, plus its two authors -- user and planner. Implementer, stabilizer and reviewer read it, never write it. - Steps 12, 14, 15 and both Definition-of-Done sections: mark ready-for-review, then REMOVE on approval rather than marking done. - Step 15 gains a guard: before removing an entry, confirm the WHY actually survives elsewhere. If it exists only in the WAL, it is not distilled yet and removal destroys it. - WAL MOTIVATION EXAMPLES becomes WHERE THE WHY GOES: a table splitting content across WAL / commit message / spec, with examples of a tracker line versus a write-up. - planner.md: WAL.md moved from its forbidden list to its write scope. The file previously forbade the planner from writing the very document it is now one of two authors of -- a direct contradiction of this convention. - review.md and reviewer.md: 'persistent journal entry' rewritten, with the reviewer told to check the WHY against the commit message and spec rather than the WAL. ## WAL cleanup, applying the new rule to itself Removed the two merged entries. Before deleting, verified their rationale survives: the toolchain policy is in spec/004_stack.md, the ledger discipline and pyright gap are in AGENTS.md, the deferral reasons are in .flake8's comments, and the assert-to-test reasoning is in the test's docstring. One gap surfaced and was fixed rather than lost: the '# nosec B105 # reason' syntax rule existed only in a commit message, so it is now in AGENTS.md STATIC ANALYSIS. That is the step-15 guard working on its first use. Also promoted the whitespace-normalisation work out of a merged entry's prose into its own planned entry -- it was forward-looking work buried inside a completed step's write-up -- and reordered entries into execution order.
tests/test_imagery_search_multi.py sat at the tests root with 23 test functions that had never executed. make test invokes pytest three times with explicit tier paths, which override testpaths in pytest.ini, so a file at the root is collected by neither. Suite count goes 418 -> 441. ## What the tests found once they ran Four failed immediately. Three shared one cause, and it was the interesting one: the harness built table rows of width SEARCH_ID_COLUMN_INDEX + 1, which is one column short of LOCAL_INDEX_COLUMN. metadataTable.item() then returned None, .text() raised AttributeError, and get_local_image_indices swallowed it into an empty list -- so every provider, zoom and min-area validation was skipped. That did not merely break three tests. It made their passing counterparts vacuous: test_same_image_provider_no_error asserted 'no error' and passed because the validation never ran at all. Fixing the row width turned three failures into passes and, more importantly, made the green ones mean something. Worth noting for the handler-narrowing step: the reason this stayed silent is provider_service.py catching (AttributeError, KeyError) around the whole lookup. A narrower handler would have made it loud. ## Tier: qgis, not functional Most cases are mock-driven and would fit the functional tier, but duplicate_imagery_search builds a real in-memory QgsVectorLayer and real QgsFeatures. QgsFeature.setGeometry rejects a MagicMock, and a real QTableWidgetItem has no setData.call_args_list to inspect. Both were fixed by using real objects: a real QgsGeometry for the AOI, and reading the stored value via item.data(Qt.DisplayRole) instead of mock call history -- which is the better assertion regardless, since it checks state rather than calls. The suite is kept in one file rather than split across tiers. Both tiers run in the same image, so a split would buy no runtime and separate tests that belong together. ## Guard against recurrence Adds tests/functional/test_tier_layout.py, which fails if any test_*.py sits at the tests root. This cost 23 silently-dead tests for an unknown period; a ten-line guard makes the next occurrence fail loudly with the filename. ## Verification - agent-make test: 17 functional, 426 qgis, UI harness green - agent-make lint: exit 0
…path
Removes E722 from the .flake8 ledger by giving every bare `except:` an explicit
exception set, plus a trailing `except Exception as e` that logs, so an
unexpected failure is visible instead of being silently treated as the expected
one. Nothing newly escapes: this is a Qt plugin, where an uncaught exception
reaches the event loop.
## The bug this uncovered
provider_service.duplicate_model_options recovered from failure with
self.aapp_context.llow_enable_processing[key] = True
The `a` had migrated out of `allow` into `app_context`. No such attribute
exists, so the handler raised AttributeError on its first iteration and never
reached startProcessing.setEnabled(True): the user saw "Duplication failed on
copying model options" and was left with the start button disabled -- exactly
the stuck dialog the recovery exists to prevent.
It survived because the recovery block was copy-pasted into every duplicate_*
step and only one copy was corrupted. Extracting _abort_duplication() fixes it
by construction and removes the drift. A fifth copy turned up in
duplicate_data_provider during a mutation test and now routes through the same
helper.
## Verification
Mutation-tested rather than assumed: reintroducing the exact typo fails all
five new tests. Without that check the tests could have passed for the wrong
reason, which is the failure mode the last MR ran into.
## Narrowing, site by site
- geometry (4): everything bottoms out in qgis_processing.run, so
QgsProcessingException plus KeyError for a result without 'OUTPUT'. fix_geoms
became a loop over the two repair methods -- the nested try/except duplicated
the two-handler shape, and `except: pass` would have tripped bandit B110.
- provider_service (5): DUPLICATION_FAILURES covers stale stored processings
(AttributeError/KeyError/TypeError/IndexError); anything else is a bug and is
logged.
- http (2): ValueError covers json.loads and AttributeError a payload that is
not an object. get_error_report_body keeps a broad catch on purpose --
error_message_parser is caller-supplied, so there is no expected set -- but a
parser that raises is now logged rather than degrading every error to
'Unknown error'.
- mapflow login_basic: ValueError alone covers all three malformed-token cases,
since binascii.Error and UnicodeDecodeError both subclass it, as does the
unpack when the decoded text has no ':'.
- schema/catalog: ValueError/TypeError are exhaustive for an Enum lookup by
value. Deliberately no log guard -- that module has no QGIS imports and must
stay functional-tier testable.
## A trap worth recording
Importing alert_service at module scope in geometry.py re-enters the circular
chain in tests/functional/conftest.py and broke collection of the whole
functional tier. It is imported lazily inside the log helper instead. The same
graph is why the new tests patch via the module object rather than a dotted
string: `mapflow.functional` is not yet an attribute of `mapflow` when patch()
walks the path.
## Scope
Bare excepts only. The 38 `except Exception` sites are untouched: no tool gates
them, and 25 are in processing_service.py and mapflow.py, which the refactoring
will rewrite. Deferred until that plan exists.
## Verification
- agent-make lint: exit 0 with E722 removed from the ledger
- agent-make test: 22 functional, 426 qgis, UI harness green
- no bare `except:` remains in mapflow/
An exception escaping plugin code previously had two fates, and both lost the bug report: it reached Qt's event loop and QGIS showed its raw "unhandled exception" dialog, which users dismiss; or a broad handler wrote it to the log panel, which is closed by default. Neither reaches us. Adds mapflow/error_guard.py and get_exception_report_body(), giving internal failures the same ErrorMessageWidget "Send a report" path that HTTP errors already had -- with the traceback pre-filled in the mail body. Wired at Http.response_dispatcher rather than per-callback. Every async response in the plugin passes through that one method, invoked from Qt's event loop via response.finished, so guarding there covers every network path at a single site instead of needing a decorator on each callback. Design notes: - report_unexpected_error never raises. A reporting path that can fail would replace the original exception with its own, destroying what it was meant to report; the dialog half is wrapped and degrades to a log line. - The mail body is percent-encoded. A raw & or # in a traceback would terminate the mailto body parameter and silently truncate the report. This also fixes the pre-existing HTTP report path, which had the same hole. - Tracebacks are capped at 40 lines, truncated from the FRONT: mail clients cut long URLs, and the tail frames are where the failure actually is. - Only the request path is included, never the full URL -- query strings carry ids and tokens, and this text goes into mail the user sends us. - Expected exceptions are untouched. Surfacing those through a report dialog would train users to ignore it. Verification: agent-make test 461 (35 functional, 426 qgis); agent-make lint exit 0. Tests cover that the guard logs exc_info, that it survives its own dialog failing, and the encoding and truncation rules.
Three comments added earlier in this branch explained what the code no longer
does, rather than justifying what it does. A comment is read by someone
opening the file for the first time, who cannot see the state being contrasted
against, so those read as narration -- and they rot on the next change.
error_message_widget: deleted. It reassured the reader that a module-level
logger needs no lazy import. Normal code needs no defence; silence says it
better.
http._format_email_body: kept why the body must be percent-encoded (a raw &
or # terminates the mailto parameter and truncates the report), dropped the
clause comparing it to the previous hand-rolled join.
log_config: rewritten rather than deleted, because the knowledge is real and
still load-bearing. Turning it forward-facing keeps it useful and true:
before The previous helper lived in functional/service/alert_service.py,
which made it unreachable from schema/catalog.py.
after Do not move this behind a helper in functional/service/ -- that
package sits on the circular import chain documented in
tests/functional/conftest.py, which would force lazy-import shims
at every call site.
All other comments this branch added were reviewed and kept: each justifies
something a reader could otherwise "clean up" and break -- a deliberately
broad except, a lazy import, narrow exception tuples, a threshold, and the
deliberate absence of a log call in catalog.py that keeps it QGIS-free.
Comment-only; agent-make test 461 passed.
Adds "Comments Describe The Code, Not Its History" to instructions/delivery.md and a fourth row to the AGENTS.md WHERE THE WHY GOES table, so the routing rule is complete: WAL tracks what is planned, the commit message carries why the change was made, spec/ holds decisions that outlive the step, and a code comment justifies what is surprising about the code as it stands. The failure mode this prevents: an agent that has just changed something comments on the delta rather than the result, producing text that only makes sense to someone who saw the previous version. Over a few PRs the code fills with reassurance that nothing strange is happening. The operative test is deliberately mechanical enough to apply under review: delete the comment, then ask whether a competent reader could now make a change that reintroduces a real problem. Yes keeps it, no deletes it. Framed to protect meaningful comments rather than discourage commenting. The section leads with the cases that MUST be commented -- broad except, lazy import, narrow exception tuple, deliberate absence of a guard, non-obvious threshold, ordering dependency, external-behaviour workaround -- before the prohibitions, because the risk of a rule like this is over-correction into silence where justification was load-bearing. Includes the "turn history into a constraint" pattern with the log_config rewrite from the previous commit as the worked example: when the old state was a genuine hazard, address the person about to reintroduce it instead of deleting the knowledge. Scope note: the banned openings cover a previous state OF THIS CODE. A comment like "the backend no longer sends this field" describes an external contract and stays legitimate.
Records two follow-ups for the hardening plan, in dependency order. Dedup is not a future nicety. The guard is already wired into Http.response_dispatcher, and a meaningful share of that traffic is timer driven -- processing status every 6s, template status every 15s, user status every 30s. One recurring bug in a poll callback therefore spawns a report dialog every few seconds, which is a worse failure than the unhandled exception window it replaced. Suppressing by exception signature within a window fixes it, and the count of suppressed occurrences is worth surfacing so the user reports "this happened 400 times" rather than "this happened". Widening the guard to UI entry points is listed second and explicitly blocked on the dedup, because applying it first multiplies the same failure mode across every slot instead of containing it. It is also sequenced after "Plan the refactoring": guard_entry_point needs a clear notion of what an entry point is, and mapflow.py's god object blurs that boundary today, so choosing sites now would be guesswork that the refactor invalidates.
Adds a "Manual test" section to instructions/delivery.md, wires it into the pre-merge step and the Implementation Definition of Done. The gap it closes: nothing currently tells the person testing a release build what to click. A changelog says what changed; it does not say what a regression would look like on screen, which is what a tester actually needs. The section has two halves -- new behaviour, and the regression surface with its symptom -- because the second is the one that gets skipped and the one that catches bugs. Two repo-specific reasons this matters more here than elsewhere, both written into the rule: - the test-ui tier is an empty harness whose Makefile target treats "no tests collected" as a pass, so anything under mapflow/dialogs/ has zero automated coverage, not merely thin coverage; - spec/004_stack.md pins CI to Linux + QGIS 3.28 LTR and states that macOS, Windows and other QGIS versions are covered by manual smoke testing only, so path, file-dialog and Qt-version-sensitive changes need an explicit callout. Notes live in the commit message rather than a tracked test-plan file. An append-only file would conflict on nearly every merge in this branch flow, since every branch appends to the same region, and its entries outlive reverts -- leaving a tester chasing behaviour that is not in the build. In the commit message the note stays welded to its diff, and only merged work contributes. The release checklist is then compiled with git log rather than maintained, so there is no cleanup commit to forget. "none" must be written explicitly: an omitted section is ambiguous between "nothing to test" and "the author forgot", and a tester cannot tell which. ## Manual test none - documentation only, no runtime code touched.
The guard added in 8161ca0 shows a report dialog per occurrence, and it is wired into Http.response_dispatcher, which runs on every async response. Both branches there go through call_guarded, so use_default_error_handler=False does not cover it: a callback that raises on the 6-second processings poll opens a dialog every six seconds. Stacking, not queueing, is what makes that fatal. Plugin modals open with exec(), which runs a nested event loop, so QTimer keeps firing while a dialog is up. The guard as it stood could therefore leave QGIS less usable than the unhandled exception it replaced. Failures are keyed by exception type plus the line they were raised from. The message is excluded because messages carry ids and paths, which would make every occurrence look new; the operation context is excluded because one broken line reached from two call paths is one bug. The window starts at 60s - above USER_STATUS_UPDATE_INTERVAL, the slowest poll - and doubles per report to a 30-minute cap, so a persistent failure still resurfaces a couple of times an hour instead of going silent for good. A 10s global floor covers the case per-signature suppression cannot: several different exceptions rotating through the same 6-second callback. Suppressed occurrences are counted and the count reaches both the dialog and the mail body. A traceback that fired 200 times is a timer-driven bug and a traceback that fired once is not, and the traceback alone cannot show which one you have. Logging stays unconditional - the log is the only complete record of ordering and frequency. Placed at mapflow/ root rather than functional/service/: it must be importable from error_guard, and functional/service/ sits on the circular import chain documented in tests/functional/conftest.py. The contract this establishes - no failure may produce unbounded dialogs, and expected failures never reach the report path - is recorded in spec/006_error_reporting.md, since it constrains every future entry point wired into the guard. Follow-up now unblocked and added to WAL: three call sites opt out of the default error handler purely to avoid stacked modals (mapflow.py:482, mapflow.py:3042, processing_api.py:89), which hides real server errors from users. The throttle removes the reason for that workaround. ## Manual test New behaviour - make a polled callback fail repeatedly, e.g. raise unconditionally in ProcessingService.get_processings_callback, then open a project so the 6s poll runs: - exactly one report dialog appears, and QGIS stays usable; before this change a new dialog appeared every six seconds and could not be outrun - leave it failing for ~2 minutes: a second dialog appears saying "This has happened N more time(s) since the last message", and "Send a report" contains a Repeated: line - make two different callbacks fail on the same poll: dialogs are spaced ~10s apart at minimum, not one per failure Regression surface: - a single one-off unexpected failure must still open its dialog immediately, with no "This has happened" line and no Repeated: line in the report body - HTTP error dialogs (report_http_error) are untouched and are NOT throttled - trigger a server error from a button press twice in a row and confirm both still show, since those are user-paced and suppressing them would hide a second, different failure - the guard must still not swallow anything: the QGIS log panel gets a full traceback for every occurrence, including the suppressed ones
Found by manual testing of the throttle: with /user/status failing, the plugin issued four requests a second indefinitely while looking healthy on screen. app_startup_user_update_timer re-asks for /user/status every 500ms because the plugin cannot configure itself without that response. The only thing that stopped it was a stop() call at the end of set_processing_limit, roughly thirty lines into the callback. That callback is invoked through the error guard, which swallows the exception and skips everything after the raise point, so a malformed payload meant the stop never ran. The error branch was worse: with use_default_error_handler=False and no error_handler, a genuine outage retried forever with nothing recording it. logout() did not stop this timer either, so the polling outlived the session it belonged to. The guard did not create the loop - the same loop existed before it - but by absorbing the exception it removed the only signal that the loop was running. That interaction is why this lands with the throttle rather than after it. Four changes, each closing one way out: - the stop happens before the configuration runs, not after it. Cleanup a callback owns must precede anything that can raise, because a guarded callback is interrupted, not completed. Recorded in spec/006_error_reporting.md, since widening the guard to more entry points multiplies this shape. - a tick is skipped while a request is in flight. Ticks were never synchronised with responses, so a slow server accumulated one outstanding request per 500ms. - the retry count is bounded, and giving up is a latched terminal state rather than an implication of the stopped timer. A test caught that distinction: without the latch the give-up warning fired once per call instead of once. - /rasters/memory moved out of the retry tick into the success path. It is a startup storage quota, already refreshed by mosaicsUpdated, and it uses the default error handler - so on a real outage it was opening a modal every 500ms, stacked through nested exec() loops. That path is not covered by the report throttle. Retrying on error is kept deliberately: a single 503 at startup should not leave the plugin unconfigured for the session. WAL: gating the features whose prerequisites never arrived (launch processings, upload imagery) behind an explanation plus a Retry button is deferred until after the refactoring - it needs one owner for "is this prerequisite satisfied", and today that state is scattered across app_context fields written from inside set_processing_limit. ## Manual test New behaviour - the injection used to find this (raise in the /user/status callback via response_dispatcher) is the fastest reproduction; a blocked host or a stopped backend gives the real one: - with /user/status failing, watch network traffic: requests stop after 20 attempts instead of continuing at 4/s, and /rasters/memory is not among them at all - one warning appears when the attempts run out, not one per tick - with a slow (not failing) /user/status, only one request is outstanding at a time - log in, let the startup fail completely, log out: polling has stopped - log in again after a failed startup: retries begin again with a full budget Regression surface - this is the plugin's whole startup configuration path, so the risk is a plugin that opens but is not set up: - normal login: balance/limit label populates, model combo, provider lists, and the projects or processings table all appear as before, and the startup poll stops after the first successful response - My Imagery: the storage quota still shows on first open, and still updates after uploading or deleting an image (it now arrives from the startup callback rather than from a retry tick) - billing: check both a credits account and an area account, since setup_for_billing runs from this callback - log out and back in within one QGIS session
…ition spec/007_architecture.md is the destination; WAL section 1 is the route. Splitting them this way because the target survives the release and the step list does not. The survey changed the plan in four places, so the numbers are recorded in the spec rather than left as impressions: mapflow.py holds ~200 methods across twelve domains and 351 direct self.dlg.<widget> references. A view/ layer already exists and is bypassed - provider_service 38 widget references, data_catalog 29, processing_service 29, project_service 28. The layer is not missing, it is optional, so the rule that carries the refactoring is "a service may not touch a widget": every current violation is an instance of it, and it is mechanically checkable rather than a matter of taste. entity/status.py is byte-identical to schema/status.py apart from one relative import, and nothing imports entity/processing.py or entity/status.py at all. So the entity-versus- schema question has no answer to find - entity is a superseded home that only provider/ ever left. Two ProcessingStatus enum classes exist at runtime and their members are never equal; whether anything compares across them is called out as a check, not assumed inert. The import cycle is tighter than the conftest comments claim. They describe a four-hop chain through layer_utils and dialogs; the actual cycle is direct, schema.processing to entity.provider and back via basemap_provider. Both test tiers import the tree twice and swallow the first ImportError to get past it, so deleting those retry loops is the acceptance check for the fix. 43 of 51 QGIS-tier test files construct objects with Class.__new__(Class) and hand-set attributes. They pin internal structure, so they break the moment a method moves and cannot serve as the safety net for the extraction. That is why the behavioral tier is its own phase before any code moves, and why it asserts on four surfaces that outlive file layout - the HTTP conversation, QGIS layer state, settings keys, and widget-visible state - rather than on method names. Sequencing notes worth keeping: - narrowing the broad exception handlers stays early, against the general "defer lint work" decision. A handler that swallows everything hides a broken extraction, and the extraction phase moves code past those sites constantly. - whitespace normalisation goes last instead of immediately before the refactor cut. The refactoring rewrites most of the offending lines anyway, so normalising first is work done twice, and a whole-tree diff would collide with every in-flight branch. - wrapping entry points in error_guard becomes cheap only after the extraction, because a controller slot is then the definition of an entry point instead of a judgement call. Also removes the merged deduplicate-error-guard-reports entry. ## Manual test none - documentation only, no runtime code touched.
… enforcement Decisions taken with the user, and the reasoning that is not obvious from the outcome. Entry point keeps the name mapflow.py. What changes is its content, not its identity; renaming it would churn every import for no gain. entity/ is deleted rather than renamed, and its one live package moves into schema/. The consequence to accept openly: schema/ then holds both API DTOs and behaviour-carrying domain types, since the provider classes have real methods. That is a loose fit for the name, and still better than a second types package whose boundary has to be re-litigated - which is precisely how entity/ became a graveyard. Recorded as a trap because it is easy to get wrong: moving entity/provider/ into schema/ does NOT break the import cycle. It relocates both ends of it into one package. The cycle dies only when the provider primitives (SourceType, CRS, BasicAuth) move to a leaf module that imports nothing, so schema.processing depends on the leaf instead of on the package that imports it back. Sequenced before or with the move. Fourteen services, each defined by the one question nobody else may answer. Two splits are worth the extra module. Auth is separated from account status because credentials are consulted by nothing but Http while /user/status limits are read by nearly every service, so keeping the widely-read thing small is what the split buys. The local filter is separated from search because it computes over an already-fetched result set and issues no request, which makes it pure enough for the functional tier. Controllers are one per UI region rather than one per service, since a region drives several services. They may not call each other: cross-region effects travel as a service signal, so adding a second listener never means editing the first controller. Services already emit (DataCatalogService.mosaicsUpdated); that becomes the rule. On dialogs, the loading pattern was already correct and the diagnosis had to be adjusted: all 13 dialogs use uic.loadUiType and no pyuic5 output is committed. So the fix is not "choose Designer or code" - it is that statically-placed widgets built in Python are invisible in Designer, which is what makes the files inconsistent to edit. Structure belongs in the .ui; Python may create only what Designer cannot express, into a container the .ui defines. Custom widgets go in by promotion. View isolation ships with a test. tests/functional/test_layering.py checks the layer rules mechanically, with today's violations as an allowlist that later steps only shrink, so the rule binds new code from the first day instead of the last. A rule that is only reviewed decays invisibly - one import added to one service - and test_tier_layout.py is the local precedent for catching a structural rule with a test rather than a habit. ## Manual test none - documentation only, no runtime code touched.
Adopts the two-package split, with a sharper test than "API object versus in-app DTO":
schema/ is what crosses the network, model/ is what the plugin owns and persists locally.
The proposed phrasing has a grey zone this codebase sits in - most in-app state IS a
parsed response - so keying on the wire keeps the boundary decidable.
The codebase already marks it, which is why the split is worth having: schema/base.py
gives Serializable an as_dict/as_json for request bodies and SkipDataClass a from_dict
whose docstring says response parsing should inherit it. Exactly two modules inherit
neither. billing.py is a vocabulary parsed from /user/status and stays; processing_history
reads and writes processing_history_{project_id} in QgsSettings and moves. Providers move
for the same reason - user-configured, persisted under mapflow_data_providers.
So model/ holds two things, and that is the correct size rather than a disappointing one.
Recorded explicitly, because the tempting next move is the wrong one: wrapping
MapflowProject, ProcessingDTO, MosaicReturnSchema and ImageReturnSchema in parallel domain
classes to fill the package. All four sit directly in AppContext already; mirroring them
buys nothing and costs a hand-written mapping per type that then drifts, which is the
failure entity/ died of. A response type earns a place in model/ only once the plugin owns
state the response does not describe.
The unplanned payoff is that this retires the import cycle as a class of bug rather than
as an incident. model/ may import schema/ - a locally-owned provider legitimately builds a
request shape - and schema/ may never import model/. Today's cycle is exactly a violation
of that direction: schema/processing.py reaches into the provider package for SourceType.
With the direction stated and checked by the layering test, the fix follows from the rule
instead of being a one-off, and someone who never knew the cycle existed cannot
reintroduce it.
Consequently the cycle fix, the entity/ deletion and the schema/model split collapse into
one Phase A step: they are the same change seen three times. Phase D is then package moves
only, reclassifying nothing.
## Manual test
none - documentation only, no runtime code touched.
Phase A of the 3.7.0 refactoring (spec/007_architecture.md). Removes ~340 lines that no import reaches, so later phases move less code. The duplicate-enum question the plan flagged is settled, and the answer is that it is inert. entity/status.py is byte-identical to schema/status.py apart from one relative import, so two ProcessingStatus classes did exist at runtime, and enum members of two distinct classes never compare equal. That would be a live bug if the two could meet. They cannot: the only importer of entity/status is entity/processing, and entity/processing has no importers at all, in the plugin or the tests. mapflow/ contains no importlib, __import__ or import_module either, so nothing reaches them dynamically. The whole branch was unreachable rather than merely unused. requests/ contained a single empty __init__.py and is referenced nowhere. entity/ itself stays for now - entity/provider/ is live and moves to model/ in the next step, which is where the package disappears. ## Manual test none - the deleted modules had no importers, so no code path changes. If anything did reach them the plugin would fail to load at all, which the test suite would not miss.
…them
Phase A of the 3.7.0 refactoring (spec/007_architecture.md § schema/ versus model/).
schema/ is what crosses the network; model/ is what the plugin owns and persists locally.
The split is worth doing because the codebase already drew the line and then ignored it.
schema/base.py gives Serializable an as_dict/as_json for request bodies and SkipDataClass
a from_dict whose docstring says response parsing should inherit it. Two things did not
fit that story: the provider classes, which the user configures and which persist under
mapflow_data_providers, and ProcessingHistory, which reads and writes
processing_history_{project_id} in QgsSettings. Neither ever touches the wire. Both are
now in model/, and that is the whole of model/ - it is meant to stay small.
The payoff is that the import cycle is gone as a category rather than patched. The rule
that model/ may import schema/ and schema/ may never import model/ makes the old cycle
unrepresentable: schema/processing.py reaching into the provider package for SourceType
was exactly a violation of that direction. SourceType, CRS and BasicAuth are wire
vocabulary - they go into request bodies - so they belong in schema/provider_types.py, and
schema/processing.py now imports a stdlib-only leaf instead of a package.
Worth recording precisely, because the conftest comments had it wrong and cost someone a
wrong mental model: the cycle was never a four-hop chain through layer_utils and dialogs.
entity/provider/provider.py imported nothing from the plugin at all. The cycle came from
package initialisation - importing entity.provider.provider first runs the package
__init__, which imports basemap_provider, which imports back into the half-built
schema.processing. Both test tiers imported the tree twice and swallowed the first
ImportError to get past it. Those retry loops are deleted, and both tiers pass without
them, which is the proof the cycle is structurally gone rather than retried around.
default.py and basemap_provider.py now import the three wire types straight from
schema.provider_types rather than picking them up from model/provider/provider.py, which
re-exports them for its own use. Same objects either way; the point is that a reader of
those files sees which side of the boundary the type lives on.
## Manual test
Regression surface - this moves types that every feature touches, so the risk is an import
that only fails on a path the tests do not reach. Worth one pass through the plugin:
- providers: open settings, add an XYZ and a TMS provider with credentials, save, reopen
QGIS and confirm both survive with credentials intact - ProvidersList is the settings
round-trip most affected
- start a processing with a Mapflow provider, with My Imagery, and with a search result,
since to_processing_params builds the request body from the moved SourceType/CRS
- imagery search: run one, confirm footprints load and preview works
- open a project with existing processings and confirm the table populates - that reads
ProcessingHistory back from settings written by an earlier build
New behaviour: none. No runtime behaviour changes; every change is a move or an import
rewrite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
v3.7.0 will bear a massive refactoring and stay an LTR version after transition to QGIS4.