varnish / varnish-modules

Collection of Varnish Cache modules (vmods) by Varnish Software
Other
185 stars 86 forks source link

Updates saintmode language and aliases #218

Closed benzvan closed 11 months ago

benzvan commented 1 year ago

Removes archaic references to blacklist and replaces with more descriptive terms. Includes aliases, examples, and log messages.

method .blacklist --> .denylist method .blacklist_count --> .denylist_count

TODO: saintmode.c still uses old code and terms.

gquintard commented 1 year ago

ping @Dridi , @hermunn

dridi commented 1 year ago

I would probably have gone with something simpler, replacing the offending word with another word:

method .blacklist --> .mark method .blacklist_count --> .mark_count

No opinion on the matter besides being happy to see $Alias in action.

benzvan commented 1 year ago

@Dridi I'm open to something simpler but also wanted something descriptive. I could go either way or use sick in place of mark. The complication for me is one is a action taken on an object (verb) and the other is a value (parameter).

benzvan commented 1 year ago

Hmm...how about

method .blacklist --> .mark_for(duration) method .blacklist_count --> .count_marked() or .mark_count()

gquintard commented 1 year ago

I like

method .blacklist --> .mark_for(duration)
method .blacklist_count --> .count_marked()

Let's see if anyone complains, if not, we can merge next week.

In the meantime, could you add a vtc to test the new functions? You can just duplicate an existing one (so that the initial API is still tested), and scrub the old wording from all the original files.

gquintard commented 1 year ago

actually, I just checked and Varnish Enterprise already looked at that, and are using denylist() and denylist_count(), we should align with that instead

benzvan commented 1 year ago

I think they made the wrong decision :-D but I'm in favor of interoperability.

benzvan commented 1 year ago

(PR updated as s/black/deny/g and a new test added to duplicate the basic testing of test 01 but with the aliases)

dridi commented 12 months ago

Going for compatibility was the obvious choice, LGTM.