domainaware / parsedmarc

A Python package and CLI for parsing aggregate and forensic DMARC reports
https://domainaware.github.io/parsedmarc/
Apache License 2.0
966 stars 210 forks source link

Changed Gzip to zlib for decompress .gz files and Replace type checks with isinstance #430

Closed Kuzuto closed 9 months ago

Kuzuto commented 10 months ago

changed GzipFile to zlib. Mimecast reports gets decompressed and other .gz as well

codecov[bot] commented 10 months ago

Codecov Report

Patch coverage: 84.21% and no project coverage change.

Comparison is base (6c84cfb) 58.35% compared to head (2d6fd3c) 58.35%. Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #430 +/- ## ======================================= Coverage 58.35% 58.35% ======================================= Files 10 10 Lines 1340 1340 ======================================= Hits 782 782 Misses 558 558 ``` | [Files Changed](https://app.codecov.io/gh/domainaware/parsedmarc/pull/430?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=domainaware) | Coverage Δ | | |---|---|---| | [parsedmarc/\_\_init\_\_.py](https://app.codecov.io/gh/domainaware/parsedmarc/pull/430?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=domainaware#diff-cGFyc2VkbWFyYy9fX2luaXRfXy5weQ==) | `61.73% <82.35%> (ø)` | | | [parsedmarc/utils.py](https://app.codecov.io/gh/domainaware/parsedmarc/pull/430?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=domainaware#diff-cGFyc2VkbWFyYy91dGlscy5weQ==) | `69.35% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

0xabu commented 10 months ago

I'm not a maintainer, just a bystander here, but if it helps, here are some suggestions to increase the chance of acceptance:

Kuzuto commented 10 months ago

I'm not a maintainer, just a bystander here, but if it helps, here are some suggestions to increase the chance of acceptance:

* Give the PR a meaningful title and description.

* Don't mix multiple logical changes in a single PR; here you seem to have the GzipFile to zlib changes (for issue [Invalid reports from mimecastreport.com due to trailing \r\n on gzip data #429](https://github.com/domainaware/parsedmarc/issues/429)) and a bunch of unrelated changes from `type` to `instanceof`, which may be perfectly reasonable but have no explanation and no relation to the zlib change.

Thanks for the tips. I can see what you mean, even I added some notes about the other changes. I just got the impression the PR will be more "valid" if tests was passing. If he merges the PR #427 first, he will see my code only changes the init.py file.

AnaelMobilia commented 10 months ago

Dear @Kuzuto ,

It looks like @seanthegeek make some flake8 E721 issues. I suggest to rebase your PR on current main.

Bests regards,

seanthegeek commented 9 months ago

I fixed the type checks, and I'll change to zlib. Thanks.