Feedback message for invalid screen dimensions missing `.format()`

[FelixHenninger]

Hi @adswa, hi everyone,

I noticed a super-tiny (and really non-)issue while testing your package: The error message for an invalid screen size could use a format() to populate the placeholder in the error text (e.g. you could add .format(' '.join([str(v) for v in args.screensize])).

I would have opened a pull request (and would still be happy to do that), but after writing the code I wondered if it would make sense instead to move the validation into the argparse section, by specifying nargs=2 and maybe type=int for the screen size argument (or float, which would also do the conversion?). I think that might simplify the code, but I didn't want to just remove your validation step without checking first.
(one more thing I tried: The argparse documentation suggests that it should be possible to write metavar=('<screenX>', '<screenY>') to make the help message clearer, but apparently there's a bug with that, or at least I couldn't get it to work -- maybe you have an idea?).

Do you have a preference for an approach? (if you think this is even worth addressing) If you do, please let me know, I'd be happy to make a PR; but please feel free to make the change directly if you feel like it, though -- I wouldn't want to make a fuss out of this tiny thing.

Kind regards, and keep up the awesome work!

-Felix

[adswa]

Hi @FelixHenninger, thanks for the bug report! The format method was indeed missing, I must have missed this.

I'd be happy to have you listed as a contributor to this Github repo, please open a PR if you want to.

Personally, I'd be completely happy with adding the missing format() method to the print() statement.

This argparse behavior is really curious, and I wasn't aware of it -- today I learned, even though I don't understand this behavior. The bug appears to be due to screensize being a positional instead of a flagged argparse argument, and thus one could get it to work (a bit hacky) by turning it into a flagged argument. I found that the code snippet below would work:

    parser.add_argument(
        '--screensize',  metavar=("screenX", "screenY"),
        nargs=2, required=True,
        help="""screensize: Resolution of screen in px, should be supplied as
        1000 800 for a screen of resolution [1000, 800]. This parameter is
        necessary to correctly normalize Length, Position, and Vector similarity
        to range [0, 1].""")

If --screensize is now flagged, instead of multimatch-gaze file1 file2 X Y one would now need to specify multimatch-gaze file1 file2 --screensize X Y, but get a nicer

help-message

(multimatch_dev) ╭─adina@muninn ~/repos/multimatch on master!
╰─➤ multimatch-gaze -h                                                      2 ↵
usage: multimatch_gaze [-h] --screensize screenX screenY
                       [--direction-threshold <TDir>]
                       [--amplitude-threshold <TAmp>]
                       [--duration-threshold <TDur>] [-o OUTPUT_TYPE]
                       [--remodnav] [--pursuit {discard,keep}]
                       <datafile> <datafile>

Multimatch-gaze: Scanpath comparison in Python.
[...]

and an

argparse check.

(multimatch_dev) ╭─adina@muninn ~/repos/multimatch on master!
╰─➤ multimatch-gaze p3.tsv p30.tsv --screensize 2880                        2 ↵
usage: multimatch_gaze [-h] --screensize screenX screenY
                       [--direction-threshold <TDir>]
                       [--amplitude-threshold <TAmp>]
                       [--duration-threshold <TDur>] [-o OUTPUT_TYPE]
                       [--remodnav] [--pursuit {discard,keep}]
                       <datafile> <datafile>
multimatch_gaze: error: argument --screensize: expected 2 arguments

Thus, changing the code to perform sanity checks with argparse given the bug would require a bit more work (tests that test cmd-line usage will blow up because --screensize will be missing, documentation would need to be adjusted). This would be a lot more work than this small gain would justify, in my opinion, so I'd suggest the format() fix.

(With one exception: If you happen to have an eager student who wants to learn how to do their first pull request and look into the basics of documentation, argparse, and unit testing, this would be a wonderful task to learn with. Having started from not knowing anything about Python or proper Python packages myself, a task like this would have taught me a lot in the beginning, I think. So if there's someone you'd think could benefit from such a task, send them to this issue, I'd be happy to guide them through it from afar -- else I'd appreciate the addition of a missing format() 🙂 )

Thanks, and cheers!
Adina

[FelixHenninger]

Hej Adina, thanks for your super-kind reply, that makes total sense, and sounds great!

If you happen to have an eager student who wants to learn how to do their first pull request and look into the basics of documentation, argparse, and unit testing, this would be a wonderful task to learn with.

That's a fantastic idea! I don't have anyone in mind immediately, but I'll ask around (I don't have many Python folks in my department, sadly), and I'm super-happy to leave this here as a good first issue for someone to join the project (maybe for Hacktoberfest?).

[adswa]

Sounds good!