traviscross / mtr

Official repository for mtr, a network diagnostic tool
http://www.bitwizard.nl/mtr/
GNU General Public License v2.0
2.6k stars 334 forks source link

Github actions added to perform lint and compile #481

Closed darless closed 11 months ago

darless commented 1 year ago

In the test folder, there was lint.sh, but I think flake8 is a better tool. When this PR is reviewed, let's discuss if we can use flake8 in which case I can remove lint.sh.

Added a compile job for linux, this runs compilation as defined in the README, runs a sample mtr to 1.1.1.1 and runs cmdparse.py test. Need documentation on what other tests to run and whether to include the testing in tox instead of running them individually.

Not adding in this PR support for cygwin, macos, or freebsd, as that will need investigation on how to run.

Minor:

Actions will run when this gets merged in.

The actions ran in my fork from this commit: MTR github actions run

Items to discuss in PR prior to merge

darless commented 1 year ago

@rewolff let me know what you think about this PR and if you would like to see any changes.

rewolff commented 1 year ago

Sorry, kind of busy with work lately.

Anyway... For the other Unix-like OSes, can't we just enable them "unmodified" from Linux and see what happens?

Apparently I can install flake8. So now I have that tool. If it replaces "lint", should it report about style discrepancies/errors in the code? and/or report suspicious constructs? e.g. a = b==1?2:5; (that's probably meant to assign 2 or 5 depending on if b equals one, but I think the compiler assigns true (1) or false (0) depending on b equalling two (1?2:5 evaluates to two)).

darless commented 1 year ago

@rewolff The linting is only running on python files which only exist in the test directory.

$ find . -name '*.py'
./test/cmdparse.py
./test/param.py
./test/probe.py
./test/mtrpacket.py

The PR contains a flake8 selects:

Ignores F821 globally: Undefined name

@rewolff Not sure what you mean for the other OSes (unmodified). The reason why i'm saying it would be a different PR, is a different OS (windows-latest or macos) likely has different way of installing the packages that are needed and likely will take much longer to run as an action. I don't think it would be worth the time to add more to this PR than what base testing needs.

darless commented 11 months ago

@rewolff what else is needed for this PR to be merged?