Skip to content

feat: Allow IDs or Models to be passed to all publishing APIs#564

Open
kdmccormick wants to merge 3 commits intomainfrom
kdmccormick/core-1-ids-publishing
Open

feat: Allow IDs or Models to be passed to all publishing APIs#564
kdmccormick wants to merge 3 commits intomainfrom
kdmccormick/core-1-ids-publishing

Conversation

@kdmccormick
Copy link
Copy Markdown
Member

@kdmccormick kdmccormick commented Apr 27, 2026

Part of: #562

To reduce noise in the implementations, I'm proposing these general patterns:

  • Keep the param positional so that callers don't care if it's model, model_id, or model_or_id.
  • When the function just needs the ID, name the param model_id and immediately invoke model_id = get_model_id(model_id).
  • When the function needs the model instance, name the param model and immediately invoke model = get_<model>(model).

If this looks good, I'll apply it to the other applets as well.

BREAKING CHANGE: Some API functions which previously accepted int now accept <Model>.ID. Runtime behavior has not changed, but callers may need to use additional annotations or typing.cast(...) in order to get their typechecker to pass.

Comment on lines +82 to +83
if isinstance(learning_package_id, LearningPackage):
return learning_package_id
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I know this is funny :P But it makes the rest of the code simpler.

created_by: int | None,
*,
dependencies: list[int] | None = None, # PublishableEntity IDs
dependencies: Sequence[PublishableEntity.ID] | None = None,
Copy link
Copy Markdown
Member Author

@kdmccormick kdmccormick Apr 27, 2026

Choose a reason for hiding this comment

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

list is invariant (its items must be the exact type, not subclasses thereof), whereas Sequence is covariant, thus more likely to behave with mypy the way consumers expect.

@kdmccormick kdmccormick requested review from bradenmacdonald and ormsbee and removed request for bradenmacdonald April 27, 2026 17:15
@kdmccormick
Copy link
Copy Markdown
Member Author

@bradenmacdonald Could you review? If you like it, I'll do the other applets this way too.

@kdmccormick kdmccormick force-pushed the kdmccormick/core-1-ids-publishing branch from 1a96f07 to 2be18fa Compare April 27, 2026 19:17
@bradenmacdonald
Copy link
Copy Markdown
Contributor

@kdmccormick I like it!

Could you please add a test for the get_model_id() helper that uses assert_type to verify that it's returning a typed ID like LearningPackageID correctly? It is working perfectly already; I just want to ensure that it stays working and doesn't downgrade to returning typing.All in some future refactor.

@kdmccormick kdmccormick force-pushed the kdmccormick/core-1-ids-publishing branch from 2be18fa to 62507c8 Compare April 28, 2026 19:58
@kdmccormick kdmccormick changed the base branch from kdmccormick/core-1 to main April 28, 2026 19:59
@kdmccormick kdmccormick force-pushed the kdmccormick/core-1-ids-publishing branch from 62507c8 to eb725ab Compare April 28, 2026 20:23
@kdmccormick kdmccormick force-pushed the kdmccormick/core-1-ids-publishing branch from 50fff8e to cb7742e Compare April 28, 2026 20:31
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