netplot icon indicating copy to clipboard operation
netplot copied to clipboard

Wheel layout is incorrect and all the files should be contained within netplot subdir

Open KOLANICH opened this issue 3 years ago • 8 comments

KOLANICH avatar Jun 27 '22 17:06 KOLANICH

Could you please state what would be the correct layout and why the current layout is a problem?

fedeb95 avatar Jun 27 '22 17:06 fedeb95

git mv config netplot/config
git mv processor netplot/processor 
git mv provider netplot/provider
git rm __init__.py

, then fix imports and paths in the packages.

setup.py should be converted into pyproject.toml using setuptools_py2cfg and then ini2toml tools. Then packages = [...] should be removed and tool.setuptools.packages.find section should be added with include=["netplot", "netplot.*"] and scripts key should be added into project section, with the appropriate changes within __main__.py. See https://peps.python.org/pep-0621/ for more info.

I guess netplot.sh is just a demo, so we don't take any steps to make it installed, but it should be changed to

sudo tcpdump -i $1 -w $2 && sudo netplot -i $1 -f $2 "${@:3:10}".

why the current layout is a problem?

It bloats the python modules dir with packages with very generic and non-unique names. It makes this package absolutely unsuitable for installation into the system.

KOLANICH avatar Jun 28 '22 04:06 KOLANICH

I'm not very experienced in python packaging but I will try to work on this as soon as possible. Feel free to work out a PR if you'd like

fedeb95 avatar Jun 29 '22 12:06 fedeb95

Currently I have no plans to do it. Also I think it'd be much better if you had done it yourself, because there are some info only 6he project author can fill in properly.

Also, looking at https://github.com/KOLANICH/python_project_boilerplate.py may be useful for you. This is a template I use for my python projects.

KOLANICH avatar Jun 29 '22 16:06 KOLANICH

I read some documentation and your previous comment and I think I've fixed the project structure now. Running a python -m pip install . works. Let me know if the new structure looks ok to you, if you have the time. Thanks

fedeb95 avatar Jun 30 '22 20:06 fedeb95

  1. setup.cfg is considered obsolete, probably you should convert it into pyproject.toml using ini2toml tool.
  2. One should NEVER bind dependencies versions using == or < constraints, I have even created a tool to undo this kind of sabotage: https://github.com/KOLANICH-tools/unpin.py
  3. I prefer to populate version using setuptools.scm. Usually when one makes a release, he makes a new git tag for it. setuptools_scm allows to only make a git tag, and the version field in metadata is automatically populated from it.
  4. provider__init__.py seems to be unneeded.
console_scripts =
netplot = netplot.netplot:main

the name of the file implementing the main CLI for the package should be __main__.py and so the entry point should look like netplot = netplot.__main__:main. And that file should end with

if __name__ == "__main__":
    main()

It will allow to call this tool also as python -m netplot.

from netplot.processor.process_processor import ProcessProcessor

It's a violation of DRY principle. The imports should be relative (must not contain the name of the paciage). It will make refactoring and forking easier.

KOLANICH avatar Jun 30 '22 21:06 KOLANICH

  1. just packages = find: will put test dir into the built wheel. It is not what you want. One should limit the scope of automatic package discovery with
[tool.setuptools.packages.find]
include=["netplot", "netplot.*"]

KOLANICH avatar Jun 30 '22 21:06 KOLANICH

I think I have addressed all the points you've made, but for the netplot.processor.processor import, it seems to be the only way that makes GitHub Actions work. Running python -m unittest discover locally goes well, but then the action fails. I will investigate the issue further some other time, in the meanwhile I think I'll stick with the unnecessary absolute import.

fedeb95 avatar Jul 01 '22 16:07 fedeb95