Skip to content

7 - API (feat): Unify Proposal Number#233

Draft
RobertRosca wants to merge 9 commits into
refactor/remove-global-statefrom
feat/unify-proposal-number
Draft

7 - API (feat): Unify Proposal Number#233
RobertRosca wants to merge 9 commits into
refactor/remove-global-statefrom
feat/unify-proposal-number

Conversation

@RobertRosca

Copy link
Copy Markdown
Member

PR no. 7 of the big refactor.

Proposal numbers are kinda confusing throughout as they are untyped str/ints which are used throughout the code for e.g. GraphQL input, filesystem paths, keys, etc..., all of which currently does validation/formatting ad hoc wherever needed.

This makes it hard to hard what a 'proposal number' is or what it's intended to be, especially with strings that colud be one of '1234', '001243', 'p001243', 'p1234'.

This PR tries to help with this by adding one ProposalNumber value type (plus a matching ProposalNo GraphQL scalar) which is then used across the slices. Adds ADR-004 for the reasoning.

Details

ProposalNumber + ProposalNo

ProposalNumber (in shared/models.py) is a pure int subclass that:

  • validates the range (1-999999) and rejects floats
  • formats to the canonical p{n:06d} form via __str__ (e.g. p001234)
  • carries a pydantic core schema so it validates/serialises in models

This replaces the old Annotated[int, Field(...)] alias, and drops the duplicate ProposalId alias that was the same thing under a different name.

ProposalNo is the strawberry scalar wrapping it (serialises to a plain int), so the value type is used at the graphql edge too.

One gotcha worth calling out: since str(proposal) now gives p001234 rather than 1234, anywhere that built a string from the number needed a look - the MyMdC client now formats it as an integer explicitly when building API URLs, otherwise the p-prefixed form would leak into the requests.

TBH I'm not set on this design, was just curious about how well a weird custom int subclass would work for this. A saner and clearer alternative would be not screwing with __str__ for the subclass and instead adding some explicit .as_str() method which returns the prefixed/padded proposal number string.

Path resolution

get_damnit_path now raises ProposalNotFoundError (ADR-001) instead of a bare RuntimeError when a proposal directory can't be found.

ADR-004 is included here but it's only half-landed (the usual pattern by now): it describes both the pure value type and an injectable ProposalPathLocator that owns the slow GPFS number-to-directory resolution behind a swappable seam. Only the value type half is in this branch - the locator (async resolution, off-event-loop timeouts,
GPFS vs fixed implementations) is a later branch. Path resolution still lives in get_damnit_path reading settings for now.

ty

This branch adds ty to the lint group since I... accidentally added it.

Tests

  • test_models.py covers ProposalNumber validation and formatting
  • new tests cover ProposalNo scalar rejection and the path-not-found case
  • the gql parity snapshot (schema.graphql) is updated for the new scalar

Stack created with GitHub Stacks CLIGive Feedback 💬

@CammilleCC

CammilleCC commented Jul 13, 2026

Copy link
Copy Markdown
Member

We might have to get ready for a bit more generalization. From the ongoing Petra III integration, it seems that they have proposals as strings, here: https://github.com/European-XFEL/DAMNIT/blob/bb53ce207593fff57c41716f8c4c435cee271e75/damnit/ctxsupport/find_beamtime.py#L7

Comment thread api/pyproject.toml
lint = [
"pyright>=1.1.406",
"ruff>=0.7",
"ty>=0.0.42",

@CammilleCC CammilleCC Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we use pyright AND ty? Might make more sense to choose one over the other. Also please include the chosen type checker in the CI and pre-commit.

@RobertRosca
RobertRosca force-pushed the refactor/remove-global-state branch from c90b2c7 to afef36c Compare July 17, 2026 06:20
@RobertRosca
RobertRosca force-pushed the feat/unify-proposal-number branch from 3512097 to 71ac1a4 Compare July 17, 2026 06:20
@RobertRosca
RobertRosca force-pushed the refactor/remove-global-state branch from afef36c to 102d22a Compare July 17, 2026 06:21
@RobertRosca
RobertRosca force-pushed the feat/unify-proposal-number branch 2 times, most recently from 5876528 to 2e7dc44 Compare July 17, 2026 08:40
@RobertRosca
RobertRosca force-pushed the refactor/remove-global-state branch from 102d22a to 3dfd6d2 Compare July 17, 2026 08:40
@RobertRosca
RobertRosca force-pushed the feat/unify-proposal-number branch from 2e7dc44 to 9dd16b1 Compare July 17, 2026 09:09
@RobertRosca
RobertRosca force-pushed the refactor/remove-global-state branch from 0873ce9 to dd5d976 Compare July 17, 2026 09:11
@RobertRosca
RobertRosca force-pushed the feat/unify-proposal-number branch from 9dd16b1 to 5358df4 Compare July 17, 2026 09:11
@RobertRosca
RobertRosca force-pushed the refactor/remove-global-state branch from dd5d976 to 1d7fdf0 Compare July 17, 2026 09:12
@RobertRosca
RobertRosca force-pushed the feat/unify-proposal-number branch from 5358df4 to 44d3d84 Compare July 17, 2026 09:12
@RobertRosca
RobertRosca force-pushed the refactor/remove-global-state branch from 1d7fdf0 to bd160c7 Compare July 17, 2026 09:12
@RobertRosca
RobertRosca force-pushed the feat/unify-proposal-number branch 2 times, most recently from 87581d5 to f7960cc Compare July 17, 2026 10:46
@RobertRosca
RobertRosca force-pushed the refactor/remove-global-state branch from bd160c7 to 15f9109 Compare July 17, 2026 10:46
@RobertRosca
RobertRosca force-pushed the feat/unify-proposal-number branch from f7960cc to f77a5ad Compare July 17, 2026 11:01
@RobertRosca
RobertRosca force-pushed the refactor/remove-global-state branch from 15f9109 to 5c8c396 Compare July 17, 2026 11:01
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.

2 participants