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

Monkeypatch core.logic to make sets work #172

Merged
merged 1 commit into from
Nov 20, 2016
Merged

Monkeypatch core.logic to make sets work #172

merged 1 commit into from
Nov 20, 2016

Conversation

arrdem
Copy link
Collaborator

@arrdem arrdem commented Nov 10, 2016

This changeset tweaks the driver implementation to use Var.setRoot in a try/finally to establish an execution scope with a patched implementation of tree-term? that won't allow infinite recursion on sets.

Criticisms of this patch:

  1. Blatant monkey patching
  2. Assumes that kibit is only ever used via the driver interface, never as a library.

More of an effort could have been made to keep the test impact minimal, but that can be cleaned up.

Fixes #78

@danielcompton
Copy link
Member

danielcompton commented Nov 10, 2016

While I don't love monkeypatching as a general rule, no-one (including me) has provided a better option here, and not handling sets has caused so much trouble for people that I doubt that Kibit is all that useful as a library currently. I'll wait a few days before merging in case there are some good reasons not to merge.

@arrdem
Copy link
Collaborator Author

arrdem commented Nov 10, 2016

I'm not really a fan of monkey patching either, but if we're gonna monkey patch this is the nicest way we could possibly do it because it doesn't leak our patched predicate. I would rather see someone take a deep dive into core.logic and figure out what's going on here / how to support sets "properly", but I can't volunteer that kind of time.

@arrdem
Copy link
Collaborator Author

arrdem commented Nov 10, 2016

Posted a new diff. Removed irrelevant diff to the test suite, factored out a with-monkeypatches macro which may be more generally useful in the future.

@arrdem arrdem merged commit 518ff1f into master Nov 20, 2016
@arrdem
Copy link
Collaborator Author

arrdem commented Nov 20, 2016

@danielcompton can we get a release whenever you get around to it?

@danielcompton
Copy link
Member

lein-kibit 0.1.3 released. Thanks for your help!

@arrdem
Copy link
Collaborator Author

arrdem commented Nov 21, 2016

@danielcompton It's a janky workaround, but this may be worth a clojure@ patch ANN since it fixes a long standing bug.

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