Changes colors to be an int, not a string

[sam-lunt]

This fixes #678 and #649

I edited the file on my system and confirmed that this fixes the issue.

[Iiridayn]

"If no type is provided, the type of the default value is used. If no default value is provided, the type is assumed to be STRING." https://click.palletsprojects.com/en/8.0.x/options/#basic-value-options - so, probably a bug on their part to overlook flag_value sorta is the default option; possibly lost in a refactor.

Also fixes #649, since #679 is a duplicate - not sure if this line will help link things.

[sam-lunt]

@Iiridayn thanks, I missed the existing issue. I edited the description so that both will hopefully be closed by this PR.

I agree that this seems like an oversight in click's logic. It's pretty unexpected that flag_value=256 gets coerced into '256'.

[tony]

Is there any assurance this PR, or #649, won't break earlier click versions?

[tony]

Does this work with click 7?

[sam-lunt]

@tony yes, the type= keyword arg does exist in Click 7.x. See documentation for click.option decorator, which forwards all keyword args to click.Option class.

[tony]

@sam-lunt Since we lock click at <8 as of a few months ago, I wonder if this is worked around via our pinning of click?

Is this still happening if you uninstall and reinstall?

Thanks for the links, and pardon my fogginess at their API docs, they had a "soft recalled" release in their API docs for a few years and that kind of challenged my trust with click. :P By the time I adopted click is was too late to remove argparse.

(Seriously considering just using argparse again, but that's for another time)

[sam-lunt]

@tony I'm using Arch Linux, which uses Click 8, despite the pin. That said, after upgrading to tmuxp 1.8.2, libtmux 0.9.0, and click 8.0.1, I'm no longer seeing this issue. Adding a break point to /usr/lib/python3.9/site-packages/libtmux/server.py:118 shows that self.colors is now 256 instead of '256'. I'm not sure what changed, but it seems that the issue is resolved.

[tony]

@sam-lunt Thank you! Nice if this is the case!

If you'd like you can close this PR for now, pending it comes up again (if it does we can reopen)

[tony]

The reason why is I am a bit concerned about updating click to 8.0 as other Linux distros aren't using it yet. I'm concerned about compatibility.

[tony]

Thank you @sam-lunt! We will monitor this from here

[sam-lunt]

@tony I think that's a good point about Click 8, but I would suggest click>=7 instead of click>=7,<8. As you can see with Arch Linux, they just ignored the <8 part anyway. I would guess that other distros will likely take a similar approach: they won't choose to hold click back to 7.x in order to satisfy tmuxp, so it's better to ensure that tmuxp works equally well with 7.x and 8.x (it seems like you are already trying to do this, so I'm mostly suggesting updating the requirements file to match your intentions)

[tony]

re: click>=7

I will consider relaxing the constraint. I have a bit too much on plate at the moment - we are seeking maintainers if that's of interest to you: #290

Also click>=7,<9 (to prevent APIs from breaking unexpectedly)

[Iiridayn]

Click 8.0.1 fixed the flag type inference which impacted this package. https://click.palletsprojects.com/en/8.0.x/changes/#version-8-0-1. Before then, this patch was manually applied by the Arch Linux package maintainer - see https://bugs.archlinux.org/task/70841.

[tony]

@Iiridayn Excellent! 8.0.1 may have fixed this by side effect

I'm still considering loosening the click constraint as well. There are trade offs. Looser versioning isn't necessarily better. Other package managers enforce these constraints and they actually protected them from being affected (once we locked <8).

More leaner version constraints on click could potentially open the door to more issues. No fault of theirs, there's a huge amount of surface area to cover. This example with int/string here was certainly harmless enough, but had ramifications downstream

[Iiridayn]

I personally feel that strong version constraints are valuable - for upstream dependencies which follow Semantic Versioning, it's easy enough to lock to version and check when a new major version is released for incompatibility with features you care about. Overly lax or unspecified version constraints can result in software breaking post-update, as seen twice now for this package in Arch, where the constraints were ignored.

For Click 8 (8.0.1 anyway) you seem solid to approve it if you don't use any of impacted features. Certainly for at least basic operation it appears to be running fine on Arch Linux systems.

[tony]

@Iiridayn and @sam-lunt : in tmux 1.9.2 I allowed click 8.0.x

How's this work?

I'll keep an eye on issues track to see if anything breaks for other distros / users too

for now I made it <8.1. I am a bit scared to do <9 because click's implicit side effects sometimes break semver (imo)