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

HeaderMatchingFilter: do not convert user supplied tags #315

Merged
merged 1 commit into from
Oct 24, 2021

Conversation

mjg
Copy link
Contributor

@mjg mjg commented Oct 22, 2021

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
Copy link

codecov bot commented Oct 22, 2021

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%) ⬇️

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%) ⬇️

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.

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.
@GuillaumeSeren
Copy link
Collaborator

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
Copy link
Contributor Author

mjg commented Oct 24, 2021

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
Copy link
Collaborator

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.

Copy link
Collaborator

@GuillaumeSeren GuillaumeSeren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@GuillaumeSeren GuillaumeSeren merged commit 3b92bda into afewmail:master Oct 24, 2021
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.

None yet

2 participants