search

weavejester / integrant

Micro-framework for data-driven architecture
MIT License
1.24k stars 64 forks source link

Add mandatory/all methods to RefLike protocol #87

Closed clyfe closed 3 years ago

clyfe commented 3 years ago

As per #77. I also rebased on master, some extra commits show up from there.

Add mandatory/all methods to RefLike protocol. These methods allow a RefLike implementation to determine which dependencies are optional for a reflike, and which dependencies are mandatory. Also renames include-refsets? to optional-deps?.

Resolves: #77 See also: #63, #68

weavejester commented 3 years ago

Why do ref-mandatory-keys and ref-all-keys need access to the configuration?

clyfe commented 3 years ago
  1. I used "common parent" technique you described to obtain a RefMap that resolves maximally - I don't need this feature (maybe just the include-refsets? -> include-reflikes? / optional-deps? rename for consistency). Frankly, I'm mildly against it.
  2. Added the config as param as to make all the data that can be available - be available; if there's anything that's gonna be needed it will be in the config - that's all the data there is.
  3. Hope to get the reflike branch released soon.
weavejester commented 3 years ago

Added the config as param as to make all the data that can be available - be available; if there's anything that's gonna be needed it will be in the config - that's all the data there is.

If we don't have a concrete use-case for a parameter, it should be omitted.

weavejester commented 3 years ago

Thanks. You should be able to rebase on reflike now, as it's tracking master again. We need some more tests, and the commits should eventually be cleaned up so that it's ready to merge.

clyfe commented 3 years ago

I squashed the commits.

We need some more tests

We have a different interface to the same behavior. Not sure what tests I should add.

weavejester commented 3 years ago

The patch adds new protocol methods; simple tests should be added for their implementations. In addition we should extend the protocol to a test implementation, and check that dependency-graph works correctly.

Regarding the commit itself, we should split out the :optional-deps? rename into its own commit, and the commit message should not have markdown in it.

clyfe commented 3 years ago

Can we manually restart pr-commit-check-skill? Seems to have hanged.

weavejester commented 3 years ago

There doesn't appear to be a way to restart it.

Can you remove the "Related to: #77" line on the first commit, as that will be part of the merge commit.

Can you change the commit message of the second commit to:

Add mandatory/all methods to RefLike protocol

These methods allow for custom RefLike implementations with different
dependency resolution logic. Fixes #77.
clyfe commented 3 years ago

Something is wrong with pr-commit-check-skill. Anyway, we good here? Can we merge this and make a 0.9.0.alpha release?