Pcapreader() does not work with PosixPath and WindowsPath fix #3596

[tanwirahmad]

Pcapreader() works fine even if the provided filename is in PosixPath or WindowsPath type. Fix #3596

I don't know if this PR requires unit tests. If it does, then please guide me where they can be added.

[p-l-]

Thanks for this fix. I think we can (should?) use os.PathLike here. What do you think?

[tanwirahmad]

I didn't know about os.PathLike. It makes perfect sense!

[p-l-]

OK now the return type has changed, since filename might be an os.PathLike object. Also it seems that you have to specify something like PathLike[Any] (meaning either PathLike[str] or PathLike[bytes]).

[p-l-]

Anyway, not sure it was a good idea after all, since Python 2.7 lacks os.PathLike... Sorry about that :-/

[codecov]

Codecov Report

Merging #3597 (4b1e061) into master (53c764d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 4b1e061 differs from pull request most recent head 4f39755. Consider uploading reports for the commit 4f39755 to get more accurate results

@@            Coverage Diff             @@
##           master    #3597      +/-   ##
==========================================
+ Coverage   86.19%   86.21%   +0.02%     
==========================================
  Files         284      296      +12     
  Lines       64874    67082    +2208     
==========================================
+ Hits        55915    57832    +1917     
- Misses       8959     9250     +291     

Impacted Files	Coverage Δ
scapy/libs/six.py	70.55% <100.00%> (+0.17%)	⬆️
scapy/utils.py	77.27% <100.00%> (+0.44%)	⬆️
scapy/__init__.py	74.54% <0.00%> (-8.48%)	⬇️
scapy/layers/ntlm.py	44.76% <0.00%> (-5.40%)	⬇️
scapy/layers/dhcp.py	82.67% <0.00%> (-4.61%)	⬇️
scapy/layers/smb.py	32.37% <0.00%> (-2.12%)	⬇️
scapy/layers/sctp.py	95.12% <0.00%> (-1.57%)	⬇️
scapy/layers/tls/crypto/cipher_block.py	96.24% <0.00%> (-1.42%)	⬇️
scapy/contrib/automotive/scanner/enumerator.py	90.93% <0.00%> (-1.33%)	⬇️
scapy/automaton.py	74.23% <0.00%> (-1.30%)	⬇️
... and 169 more

[p-l-]

Sorry, I think it is better to come back to your first option, it was better since it would have worked with Python 2 directly, am I wrong?

[gpotter2]

Unsure this PR is really necessary: there are quite a few ways to get the full absolute path from the Path... object :/

[tanwirahmad]

@gpotter2 The purpose of this PR is to support os.PathLike objects for Pcapreader(). IMHO, they are more useful than strings based paths.

@p-l- My first option would not have worked for Python 2 because the pathlib library was added in Python 3.4

[p-l-]

@gpotter2 your call

[gpotter2]

• PR doesn't pass both PEP8 and Mypy tests
• I said scapy.compat, not scapy.libs.six because we regularly update it so any changes in there would be lost.
• PR is wrong because:
  • filename = fname is wrong in case of an os.PathLike... it's not the filename anymore
  • you can't just change the return type of filename to Union[...] just to make it pass the tests, you need to actually fix it

[gpotter2]

(As for the doc build failure: sphinx-doc/sphinx#10710)

[guedou]

I am unsure this PR is necessary.

[gpotter2]

I kinda agree. Are we really going to update every function in Scapy to support path stuff, when in one call you can get the absolute path

[guedou]

I am closing this PR as Scapy maintainers seem that it is not necessary.