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

🐛 skip set if no mail selected #286

Merged
merged 1 commit into from
Sep 12, 2021

Conversation

NCenerar
Copy link
Contributor

Summary

If no mail are selected by the strategy (with regexp for example), it could lead to message loss because it means all will be discarded.
I think that the desired behavior is to skip the set like when all mail are selected.

Preliminary checks

New Features Submissions:

  1. Does your submission pass tests?
  2. Have you lint your code locally prior to submission?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

If no mail are selected by the strategy, it could lead
to message loss because it means all will be discarded.
The set should be skipped like when all mail are selected.
@codecov
Copy link

codecov bot commented Sep 12, 2021

Codecov Report

Merging #286 (fe2d9de) into develop (23a588a) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #286      +/-   ##
===========================================
+ Coverage    77.79%   77.94%   +0.15%     
===========================================
  Files            8        8              
  Lines          725      730       +5     
  Branches       117      118       +1     
===========================================
+ Hits           564      569       +5     
  Misses         126      126              
  Partials        35       35              
Impacted Files Coverage Δ
mail_deduplicate/deduplicate.py 75.39% <100.00%> (+0.66%) ⬆️

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 23a588a...fe2d9de. Read the comment docs.

@kdeldycke
Copy link
Owner

I'm pretty sure this solves #203 right? If so then this PR seems better than #204 as it does not introduce any extra option.

@NCenerar, can you please look at #204 and double check if my speculations are right? I'm ready to merge it right away if it is the case! 🙂

@NCenerar
Copy link
Contributor Author

Well, it is not really clear what he tries to accomplish.
I think this might provide a good solution for him but in fact, I think the discard actions will be more appropriate ;)

@kdeldycke kdeldycke added the bug label Sep 12, 2021
@kdeldycke
Copy link
Owner

Good point. Thanks @NCenerar again, and let's merge this! :)

@kdeldycke kdeldycke merged commit 36dfc5c into kdeldycke:develop Sep 12, 2021
@NCenerar NCenerar deleted the fix/strategy-select-zero branch September 12, 2021 10:59
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2021
@kdeldycke kdeldycke added 🐛 bug Something isn't working, or a fix is proposed and removed bug labels Nov 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐛 bug Something isn't working, or a fix is proposed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants