-
Notifications
You must be signed in to change notification settings - Fork 82
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
Support list or string for default_command config #384
Conversation
c4f16a9
to
d69fc52
Compare
Failing tests seem to be unrelated to my change. Looks like master is failing on its own: #385 |
Hmm... True, master seems to have broken. I'm guessing some upstream lib recently changed. Lemme look into that. 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi again. Thanks for contributing! :)
I've fixed master via #387, so you can merge/rebase and CI should work. Looks like a recent change in click 7.1 broke tests.
This is a very welcome improvement, so thanks for addressing it.
Can you please update the docs too? Also, feel free to add yourself to AUTHORS
. Thanks!
f91777c
to
9d5cf7e
Compare
Rebased & test are working now. Thanks! For documentation, I'm not seeing any existing documentation for |
Documentation is rendered in the Configuring page, but is actually extracted from the file that defines these settings. That would be the right place for it. |
I got super wrapped up with other things for the past few weeks. I updated the documentation, but wasn't sure how to denote that the option could be a string or list. I think it's fair to mark it as a list since the config parsing library coerces a list of size one into a string anyway:
becomes a list
is a string. What are your thoughts? |
d0b52d2
to
5019f02
Compare
5019f02
to
b2a924b
Compare
b2a924b
to
6f6f7f5
Compare
Ugh github closed my PR for resetting my branch back to master trying to figure out why the tests are failing again |
d9e43a2
to
b2a924b
Compare
I think documenting it as a list is probably best, since that's the most flexible way of using it. Don't worry too much if the ability to use a string isn't too obviously reflected. |
It's a pity to see this PR stuck, I was considering using it to get a default list sorting ( see GH-558 ) @alexdavid are you planning to get back to this or should I consider re-filing it under my name ? |
Things like Arguments are split on space, which I guess shouldn't be an issue for real-life usages? |
I'm running |
On Thu, Jul 25, 2024 at 02:17:09AM -0700, Hugo wrote:
Things like `default_command = "list --sort due --no-reverse"` work on the latest version.
Arguments are split on space, which I guess shouldn't be an issue for real-life usages?
Indeed it works for me. I'd close this PR
…--strk;
|
Latest is v4.4.0, from Oct 2023. You can ping the Debian If that leads nowhere, try using pipx. |
Fixes #348
default_command
can now be either a string (as before):or a list which is passed directly to argv:
Let me know if you want me to add any documentation around this, and if so, where