-
Notifications
You must be signed in to change notification settings - Fork 0
feat: needs #30
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
feat: needs #30
Conversation
000dd94 to
532ac66
Compare
Signed-off-by: Henry Schreiner <[email protected]>
532ac66 to
d746f2b
Compare
Not a fan of this either. The needs are mostly controlled by the upstream right? It feels like the user might have to change the ordering depending on the upstream's implementation. The fixed ordering is mostly to avoid the topology sorting right? But would that even be necessary? I was thinking that it would sort itself dynamically, something like class DynamicSettings(Mapping[str, str | list[str] | dict[str, str] | dict[str, list[str]]]):
def __init__(
self,
static_settings: Mapping[str, str | list[str] | dict[str, str] | dict[str, list[str]]],
providers: dict[str, DMProtocols],
):
self._static_settings = static_settings
self._providers = providers
self._requesting_keys = set()
def __getitem__(self, key: str) -> str | list[str] | dict[str, str] | dict[str, list[str]]:
# Try to get the settings from either the static file or dynamic metadata provider
if key in self._static_settings:
return self._static_settings[key]
if key not in self._providers:
raise ValueError(f"{key} is not defined in any source")
# Check if we are in a loop, i.e. something else is already requesting this key while
# trying to get another key
if key in self._requested_keys:
# We are in a loop
raise ValueError(f"Encountered a circular dependency at {key}: {self._requested_keys}")
# Otherwise try to get the current item, but first mark the current key as being requested
# in order to detect when we have hit a circular dependency
self._requesting_keys.append(key)
value = self._providers[key].dynamic_metadata(key, self)
self._requiesting_keys.remove(key)
return valueProbably I have the |
|
I like this much better for scikit-build-core. For this, I'm not as sure - I don't expect backends to have to use I think what I'll do is sync this with scikit-build-core, then make a PR for the above, and we can compare. List-based has the benefit of allowing multiple return values, and running multiple times (assuming the partial dynamic metadata proposal gets accepted). This has the benefit of being easier for users (especially with the automatic "needs"). |
|
Regarding dict vs list, I am not opposed to either, I think both interfaces are neat. Just the pre-requirement to get the order correctly was my issue there. But with the automatic needs, that issue is nullified so 👍 to either. |
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
db8b2b6 to
0c7a678
Compare
This syncs up with scikit-build-core's scikit-build/scikit-build-core#1047. Closes #21.
I'd like to consider an alternative: we could make these lists. So then:
This would be guaranteed to always run in order top to bottom, so topological sorting is not needed. This would not have a nice path to standardization, though. A small downside is that a user then has to get the order correct, but it could be an error if they don't, and you could chain these (ideal for partially dynamic metadata). We could even skip the "field" field. Then the signature is a bit simpler - no "field", and the return is then a dict with the added or changed fields. That would allow one dynamic-metadata to update multiple fields, and would reduce the duplication that currently is a bit annoying without standardization.