Skip to content
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

Properly resolve query-fn #53

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Properly resolve query-fn #53

merged 1 commit into from
Jan 16, 2024

Conversation

gerdint
Copy link
Contributor

@gerdint gerdint commented Jan 16, 2024

DItto.

I don't like this extra complexity in the beginners tutorial but it is what it is.

@yogthos yogthos merged commit e46bee2 into kit-clj:master Jan 16, 2024
@yogthos
Copy link
Contributor

yogthos commented Jan 16, 2024

Yeah, it's a just a bit of extra magic. :)

@gerdint
Copy link
Contributor Author

gerdint commented Jan 16, 2024

Would have been nice if Integrant somehow did this automatically but that's the price we pay for the system definition just being a map I guess.

@yogthos
Copy link
Contributor

yogthos commented Jan 16, 2024

yup

@gerdint
Copy link
Contributor Author

gerdint commented Jan 16, 2024

@yogthos Actually, in practice the modification times could be stored as a defonced atom the kit.edge.db.sql.conman namespace instead of inside the component instance. This would preclude running several instances of a system including that component in the same REPL session, but is that goal for Kit?

I have actually never seen an example of that in docs or examples I think, but would be nice for stuff like bringing up an additional system with a test profile when running tests from the REPL. Is that suppose to work?

@yogthos
Copy link
Contributor

yogthos commented Jan 17, 2024

I'd rather not put hidden state in component libraries. It would provide a bit of convenience, but I don't think it's worth it overall. In general, I'd like to keep Kit as unopinionated as possible, and I can definitely see uses for having multiple systems for things like tests.

@gerdint
Copy link
Contributor Author

gerdint commented Jan 17, 2024

@yogthos I agree, which is obviously why I coded the patch as such.

I do find the testing page a bit light on details. Perhaps a section on such a use case could be described?

@yogthos
Copy link
Contributor

yogthos commented Jan 18, 2024

oh yeah, what if we just use metadata instead of sticking it into a map?

@yogthos
Copy link
Contributor

yogthos commented Jan 18, 2024

and yeah testing page could definitely be fleshed out more :)

@yogthos
Copy link
Contributor

yogthos commented Jan 18, 2024

@gerdint
Copy link
Contributor Author

gerdint commented Jan 18, 2024

Very sweet indeed!
I am inclinded to ping James Reeves about this trick. I feel it makes resolve-key all but redundant, and should at least be mentioned in the Integant README.

@yogthos
Copy link
Contributor

yogthos commented Jan 18, 2024

Yeah it's probably worth mentioning, this is seems like the exact use case metadata is meant for.

@gerdint
Copy link
Contributor Author

gerdint commented Jan 18, 2024 via email

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