MorseKOB / PyKOB

Python implementation of a library for Morse and MorseKOB functionality
https://sites.google.com/site/morsekob/morsekob40
MIT License
4 stars 2 forks source link

Config storage is inconsistent about treating 'Sound' as string vs. boolean #30

Closed pwdirks closed 4 years ago

pwdirks commented 4 years ago

The existing config.py code is somewhat inconsistent about the 'SOUND' option - it's stored in the config file as a string ('ON' or 'OFF') but it's retrieved and used as a boolean. I believe it always came out 'True' as a result. Pull request #29 changes the config.py code to at least consistently store only the strings 'ON' and 'OFF' and have the Clock.py code re-interpret it as True or False respectively when creating the KOB.

We should either have all calling code configured similarly or to change the config option to save and restore a boolean. The current usage makes it awkward to set the default option for the "--sound" override.

Another option would be to define two complementary options (e.g. "-a" for audio on and "-q" for "quiet"?) argsparse can nicely handle the mutual exclusivity between those options.

The main question is to settle the content of the config file, and then make everything consistent.

AESilky commented 4 years ago

The distutils.util.strtobool() processes a string value of 'YES, TRUE, ON, 1' to True and 'NO, FALSE, OFF, 0' to False. This allows a user to enter any of these as a value for the option (or manually edit them in the .ini file) and have it processed correctly into a True/False boolean for use by the code.

Here is a test using the existing code with those 8 different values specified as the option value and the result printed. Printing the result uses the boolean value in an if statement to print either 'ON' or 'OFF' (I removed the additional command output to minimize the size):

C:\Users\esilk\code\learn\MorseKOB-4_0-py>configure -a off
Sound: OFF

C:\Users\esilk\code\learn\MorseKOB-4_0-py>configure -a on
Sound: ON

C:\Users\esilk\code\learn\MorseKOB-4_0-py>configure -a false
Sound: OFF

C:\Users\esilk\code\learn\MorseKOB-4_0-py>configure -a true
Sound: ON

C:\Users\esilk\code\learn\MorseKOB-4_0-py>configure -a no
Sound: OFF

C:\Users\esilk\code\learn\MorseKOB-4_0-py>configure -a yes
Sound: ON

C:\Users\esilk\code\learn\MorseKOB-4_0-py>configure -a 0
Sound: OFF

C:\Users\esilk\code\learn\MorseKOB-4_0-py>configure -a 1
Sound: ON

Manually editing the configuration (.ini) file to:

[PYKOB]
speed=18
sound=false

Results in an output of:

C:\Users\esilk\code\learn\MorseKOB-4_0-py>configure
Sound: OFF

as expected.

AESilky commented 4 years ago

My understanding is that configparser only reads/writes string values to be compatible with the INI file format. Maybe I'm not up to date with newer options.

AESilky commented 4 years ago

I ran a test with Sample.py. It uses the boolean value directly from config.Sound:

SOUND = config.Sound # whether to enable computer sound for sounder
<<snip>>
myKOB = kob.KOB(PORT, audio=SOUND)

With configure -a 0 and configure -a yes and I didn't get sound the first time and I got sound the second time. So it seems to be working correctly.

pwdirks commented 4 years ago

You're right - it seems to work as it should. I believe somewhere I must have looked inside config.py at the string value of 'Sound' and w/o the 'strtobool' that does in fact always get mis-interpreted as True if treated as a boolean (whether it's 'ON' or 'OFF').

There's nothing needed here - works as designed.