Skip to content

WIP: streamline mill.api.Ctx usage #4794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lefou
Copy link
Member

@lefou lefou commented Mar 27, 2025

In general, we want use mill.api.Ctx instead of e.g. mill.api.Ctx.Log or mill.api.Ctx.Job when passing it implicitly to helper methods and workers. We typically need the other properties too when evolving the implementations or providing a second implementation. If we use mill.api.Ctx from the start, we can maintain a more applicable and stable API.

@lefou lefou force-pushed the streamline-ctx-usage branch from 0db3e53 to 0b684a3 Compare March 27, 2025 22:21
@lefou lefou marked this pull request as ready for review April 1, 2025 08:55
@lefou lefou requested a review from lihaoyi April 1, 2025 08:56
@lihaoyi
Copy link
Member

lihaoyi commented Apr 1, 2025

Not sure this is the right thing to do, as it means these methods would depend on a much richer API than they really need. e.g. In the test suite we have to create an entire DummyCtx just to satisfy the signature even if everything inside is unused.

@lefou
Copy link
Member Author

lefou commented Apr 1, 2025

I identified it as sometimes limiting in the past. Esp. when we added new things to the context like jobs or workspace or lately fork to the context. It typically results in binary-incompatible changes or impossible changes in the work case. The key is the fact, that a class/trait can better evolve over time. If we want to avoid this case you pointed out, we could alternatively invent a context version with all components being optional, which might be better suited for SPI (internal API) which also need to be tested in isolation.

@lefou lefou marked this pull request as draft April 24, 2025 20:29
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