afewmail / afew

an initial tagging script for notmuch mail
ISC License
325 stars 98 forks source link

HeaderMatchingFilter: do not convert user supplied tags #315

Closed mjg closed 3 years ago

mjg commented 3 years ago

fe71b20 ("lowercase generated tag names", 2014-02-18) made the HeaderMatchingFilter convert tags to lower case. While this may make sense for tags which are generated from header values it certainly does not for tags which the user specified verbatim in their config.

So, only convert the values resulting from a pattern match.

codecov[bot] commented 3 years ago

Codecov Report

Merging #315 (9290682) into master (8ef9a5b) will decrease coverage by 0.04%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
- Coverage   47.08%   47.03%   -0.05%     
==========================================
  Files          30       30              
  Lines        1079     1080       +1     
==========================================
  Hits          508      508              
- Misses        571      572       +1     
Flag Coverage Δ
unittests 47.03% <0.00%> (-0.05%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
afew/filters/HeaderMatchingFilter.py 40.00% <0.00%> (-2.11%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8ef9a5b...9290682. Read the comment docs.

GuillaumeSeren commented 3 years ago

Hey @mjg , Sorry to reply late.

I am not sure that many people use non lowercased tags, I don't and I am not finding anything close to that in notmuch doc.

If we can find that use case in notmuch, we could add this but it would also require a test.

mjg commented 3 years ago

First of all, tags are case sensitive and mixed and uppercase tags are allowed. So there is no reason to change the case of tags automatically, and every reason to keep it as is. Rightly, afew does not change the case of tags anywhere else, except in this filter.

Now, for tags created automatically from a header value, it can make sense to normalize it to lower case (though one might still argue about it). This is what the original commit intended and what my patch does not change at all.

But the original commit also converted every tag that the user specified verbatim in their config, and that is what I change. Say, you have:

[HeaderMatchingFilter.3]
header = X-Sieve-Redirected-From
pattern = mickey@disney.com
tags += DisneyInc

then afew really has no business changing the case of the tag which the user specified verbatim.

GuillaumeSeren commented 3 years ago

afew does not change the case of tags anywhere else, except in this filter.

I aggree with that, and the patch is small let's merge it.