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

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

Open
KOLANICH opened this issue Jun 27, 2022 · 8 comments

Comments

@KOLANICH
Copy link

No description provided.

@fedeb95
Copy link
Owner

fedeb95 commented Jun 27, 2022

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

@KOLANICH
Copy link
Author

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.

@fedeb95
Copy link
Owner

fedeb95 commented Jun 29, 2022

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

@KOLANICH
Copy link
Author

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.

@fedeb95
Copy link
Owner

fedeb95 commented Jun 30, 2022

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

@KOLANICH
Copy link
Author

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

  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.*"]

@fedeb95
Copy link
Owner

fedeb95 commented Jul 1, 2022

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.

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

No branches or pull requests

2 participants