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

info*, offer :var-meta-allowlist option #228

Merged
merged 3 commits into from
Mar 3, 2024
Merged

info*, offer :var-meta-allowlist option #228

merged 3 commits into from
Mar 3, 2024

Conversation

vemv
Copy link
Member

@vemv vemv commented Mar 2, 2024

Part of clojure-emacs/cider-nrepl#851

It allows consumers to specify var metadata beyond Orchard's fixed whitelist.

Cheers - V

@vemv vemv requested a review from bbatsov March 2, 2024 21:07
@vemv
Copy link
Member Author

vemv commented Mar 2, 2024

@bbatsov PR ready, the master builds are red due to Clojure 1.12

(investigating this weekend)

@vemv
Copy link
Member Author

vemv commented Mar 2, 2024

The fix was actually easy - done.

@bbatsov
Copy link
Member

bbatsov commented Mar 3, 2024

These days I try to avoid the whitelist terminology for various reasons, so let's name this something else - e.g. var-meta-keys or something along those lines. (basically something reflecting better we are selecting only a subset of the var metadata - meaningful keys, useful keys, etc) Let's also add an alias to the old var matching whatever name we pick here.

@vemv
Copy link
Member Author

vemv commented Mar 3, 2024

Good catch! Will refactor

vemv added 2 commits March 3, 2024 12:29
A clojure.core var slightly changed in `master`, so to make matters simpler, a var under our control is exercised instead.
It allows consumers to specify var metadata beyond Orchard's fixed whitelist.
@vemv vemv force-pushed the cider-nrepl-851 branch from 6e7a386 to e83b9ab Compare March 3, 2024 11:42
@vemv
Copy link
Member Author

vemv commented Mar 3, 2024

Ready

@vemv vemv changed the title info*, offer :var-meta-whitelist option info*, offer :var-meta-allow option Mar 3, 2024
@vemv vemv changed the title info*, offer :var-meta-allow option info*, offer :var-meta-allowlisy option Mar 3, 2024
@vemv vemv changed the title info*, offer :var-meta-allowlisy option info*, offer :var-meta-allowlist option Mar 3, 2024
@vemv vemv merged commit ee89432 into master Mar 3, 2024
0 of 30 checks passed
@vemv vemv deleted the cider-nrepl-851 branch March 3, 2024 18:44
@bbatsov
Copy link
Member

bbatsov commented Mar 3, 2024

I meant using something more specific than a generic name like allowlist, but anyways - that's not a big deal. The conversation around blacklist vs allowlist also made me realize how bad such names are in general, as they don't carry much meaning. In our case we want only a certain properties because we don't care about the others and they might have content that we can't parse. That's not really an allowlist, but something more specific IMO, but I guess I didn't manage to convey this clearly.

@vemv
Copy link
Member Author

vemv commented Mar 3, 2024

Ah, I see, sorry about that. Could have been selected-keys (following select-keys).

Although there's some value in using a term that is well-understood, even if it doesn't always exactly map to what it means.

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