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

feat: allow using a list as linux dev value #647

Merged
merged 13 commits into from
Dec 2, 2023

Conversation

rszyma
Copy link
Contributor

@rszyma rszyma commented Nov 29, 2023

Describe your changes. Use imperative present tense.

Closes #121

Allows to define device list as an actual list of strings.

Old way (colon separated string) is not removed for compatibility.

Checklist

  • Add documentation to docs/config.adoc
    • Yes
  • Add example and basic docs to cfg_samples/kanata.kbd
    • Yes
  • Update error messages
    • Yes
  • Added tests, or did manual testing
    • Yes and Yes

@rszyma
Copy link
Contributor Author

rszyma commented Nov 29, 2023

Before adding tests and changing docs, I'll wait for confirmation from @jtroo if we're going for this implementation, since there were a couple of other proposals in #121.

Copy link
Owner

@jtroo jtroo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall seems like a good solution

parser/src/cfg/defcfg.rs Outdated Show resolved Hide resolved
parser/src/cfg/defcfg.rs Show resolved Hide resolved
@rszyma
Copy link
Contributor Author

rszyma commented Dec 1, 2023

By the ocassion that we're changing linux device stuff, maybe it should be changed to not allow and error when more than one of linux-dev* items are defined at a time?

I don't see any benefits of them being allowed to co-exist. Both linux-dev-names-* variant are already taking care of what the opposite variant would exclude/include. And having linux-dev takes priority over both linux-dev-names*.

docs/config.adoc Outdated Show resolved Hide resolved
docs/config.adoc Outdated Show resolved Hide resolved
@jtroo
Copy link
Owner

jtroo commented Dec 1, 2023

By the ocassion that we're changing linux device stuff, maybe it should be changed to not allow and error when more than one of linux-dev* items are defined at a time?

I don't see any benefits of them being allowed to co-exist. Both linux-dev-names-* variant are already taking care of what the opposite variant would exclude/include. And having linux-dev takes priority over both linux-dev-names*.

This change doesn't seem worth potentially breaking some users' configs. Some users may have left 2 or 3 of the variants in as part of testing or some other reason and never bothered to remove it.

parser/src/cfg/defcfg.rs Outdated Show resolved Hide resolved
@rszyma
Copy link
Contributor Author

rszyma commented Dec 1, 2023

This change doesn't seem worth potentially breaking some users' configs. Some users may have left 2 or 3 of the variants in as part of testing or some other reason and never bothered to remove it.

True. I guess this could potentially go into v2.0 backlog, if we have one already.

@jtroo
Copy link
Owner

jtroo commented Dec 2, 2023

Made a change to the error condition of empty devices - technically the only guaranteed error is if linux-dev is empty. The exclude being empty is fine and include being empty might be overridden by someone's linux-dev.

@jtroo jtroo merged commit 9e5c8d9 into jtroo:main Dec 2, 2023
3 checks passed
}
}
"linux-dev-names-include" => {
#[cfg(any(target_os = "linux", target_os = "unknown"))]
{
cfg.linux_dev_names_include = Some(parse_linux_dev(val)?);
if cfg.linux_dev.is_empty() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be if cfg.linux_dev_names_include... ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that, I'll fix it in main directly

@rszyma rszyma deleted the linux-devices-list branch December 2, 2023 05:38
psych3r pushed a commit to psych3r/kanata that referenced this pull request Dec 3, 2023
This allows a user to define devices using a sexpr list.

The old way (colon separated string) is not removed for compatibility.
jian-lin added a commit to NixOS/nixpkgs that referenced this pull request Dec 30, 2023
This is better because it allows some special characters in the device
path.  See [1] for more information.

[1]: jtroo/kanata#647
marcusramberg pushed a commit to marcusramberg/nixpkgs that referenced this pull request Dec 30, 2023
This is better because it allows some special characters in the device
path.  See [1] for more information.

[1]: jtroo/kanata#647
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

Successfully merging this pull request may close these issues.

Do not use : to separate linux devices
2 participants