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

allow default card index -1 #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

orrpan
Copy link

@orrpan orrpan commented Apr 26, 2019

No description provided.

@kingosticks
Copy link
Member

Would you mind providing a bit of description in the commit messages as to what this solved.

@orrpan
Copy link
Author

orrpan commented Apr 26, 2019

If you want to set card to default, it is -1. Now there is a limit to only have positive integers.

class alsaaudio.Mixer(control='Master', id=0, cardindex=-1, device='default')

docs

@adamcik
Copy link
Member

adamcik commented Apr 26, 2019

Other option could be to leave min=0 and instead set optional=True and treat this field not being set / blank as -1 in the code (you might need to update the default config to be blank).

This IMO gives a nicer config exposed to end users than needing to know that -1 is a magical value.

Either way this is missing an update to the README documenting the new config semantics.

@orrpan
Copy link
Author

orrpan commented Apr 26, 2019

@adamcik thats a smart solution.
But when I test
schema['card'] = config.Integer(minimum=0, optional=True)
schema['card'] = config.Integer(minimum=-1, optional=True)

cardindex becomes 0, so it seems unnecessary to add further logic than having min=-1.
Yes, I have to update README.

@adamcik
Copy link
Member

adamcik commented Apr 26, 2019

https://github.com/mopidy/mopidy-alsamixer/blob/master/mopidy_alsamixer/ext.conf#L3 would need to blank, but I'm not 100% this would work. If it doesn't we can go ahead and merge what you have :-)

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.

3 participants