Re: [chirp_devel] [csv] Friendlier defaults for missingcToneFreq/rToneFreq columns in csv import - Fixes #1577
Responses inline.
On Apr 24, 2014, at 8:59 PM, Tom Hayward - esarfl@gmail.com wrote:
On Thu, Apr 24, 2014 at 8:09 PM, chirp.cordless@xoxy.net wrote:
Patch attached. -dan
I haven't tested it yet, but I see a few issues with style.
- Don't mix tabs and spaces. We prefer spaces.
Sorry, I will try to pay more attention to that. As an old-school C hacker, tabs are instinctive, but ever since some of my earlier Python attempts didn't work because of indenting issues, I've really tried to use only spaces. Guess I failed here. Do you have any hints for making this visible?
- 0 is a number. You can (and should) use True and False in Python to
represent booleans. This is a style thing, but it becomes important when another developer does a test like "1 is True". This evaluates False because the "is" operator is testing identity; 1 and True are different things.
Okay. Understood.
Here's a Python trick you may not be aware of:
_file_has_rTone = 0_file_has_cTone = 0for header in headers:- if header == "rToneFreq":
_file_has_rTone = 1- elif header == "cToneFreq":
_file_has_cTone = 1Is equivalent to: _file_has_rTone = "rToneFreq" in headers _file_has_cTone = "cToneFreq" in headers
Short and sweet. It will iterate the list twice instead of once, but I think that's acceptable for readability.
I didn't know the idiom, and certainly way more readable. I'd actually felt a little guilty about iterating the header line for every line in the file, but didn't see a way to factor that without creating globals, which I didn't think would be wanted. If doubling that again for readability is preferred, I have a better calibration of the tradeoffs expected.
You didn't ask for a patch respin, shall I, or is this OK and I should just take this as advice for future work?
Thanks and regards,
-dan
On Fri, Apr 25, 2014 at 7:56 AM, chirp.cordless@xoxy.net wrote:
Do you have any hints for making this visible?
My text editor shows whitespace when text is highlighted. I think it's configurable to have it show whitespace all the time, but this is annoying. I'm using Sublime Text.
Here's a tip for vim: http://vim.wikia.com/wiki/Highlight_unwanted_spaces
If you are super cool like Dan Smith and use emacs, you can do it this way: http://www.emacswiki.org/emacs/ShowWhiteSpace
_file_has_rTone = "rToneFreq" in headers _file_has_cTone = "cToneFreq" in headers
I didn't know the idiom, and certainly way more readable. I'd actually felt a little guilty about iterating the header line for every line in the file, but didn't see a way to factor that without creating globals, which I didn't think would be wanted. If doubling that again for readability is preferred, I have a better calibration of the tradeoffs expected.
How about this...
if not hasattr(self, "_file_has_rTone"): self._file_has_rTone = "rToneFreq" in headers if not hasattr(self, "_file_has_cTone"): self._file_has_cTone = "cToneFreq" in headers
Not global, but only checks for rToneFreq once per file.
You didn't ask for a patch respin, shall I, or is this OK and I should just take this as advice for future work?
Yes, please respin.
Tom KD7LXL
On Apr 25, 2014, at 9:28 PM, Tom Hayward esarfl@gmail.com wrote:
On Fri, Apr 25, 2014 at 7:56 AM, chirp.cordless@xoxy.net wrote: Do you have any hints for making this visible?
My text editor shows whitespace when text is highlighted. I think it's configurable to have it show whitespace all the time, but this is annoying. I'm using Sublime Text.
Here's a tip for vim: http://vim.wikia.com/wiki/Highlight_unwanted_spaces
If you are super cool like Dan Smith and use emacs, you can do it this way: http://www.emacswiki.org/emacs/ShowWhiteSpace
Just use emacs' python mode (usually the default when editing .py files). It will keep you out of trouble, consistently writing out only spaces so you don't need to look at the white space. You can use the tab key to indent, but it will actually just insert the appropriate number of spaces.
-Les
participants (3)
-
chirp.cordless＠xoxy.net -
Les Niles -
Tom Hayward