841061 - Preferences.read_prefs doesn't handle comments very well

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

There are two problems:
1) '//' at the start of the line isn't skipped: https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/prefs.py#L163
2) Comments after a preference prevent the ";" from being rstripped: https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/prefs.py#L174

Comment 1

[Jeff Hammel]

I was reading the code this weekend and, absent of any comments in the code I concluded that either A. I must have really known what I was doing; or the much more likely B. its likely this code will not meet all use cases. Now I know :/

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - handle prefs properly

It's kind of tricky as we need to figure out if the "#" or "//" characters are actually comments or part of a string. I ended up using the Python tokenizer library as a regex to accomplish this would be very very tricky.

Comment 3

[Jeff Hammel]

will review in full soon, but just wanted to note:

+import cStringIO

Last I checked (though it could have been fixed in the several years), cStringIO had a problem with unicode.  Is this still true?  Whether using StringIO or cStringIO, I have traditionally done:

from StringIO import StringIO
# or
from cStringIO import StringIO

so that if the library needed to change, it would be easy.

If cStringIO is "infalliable" now, then I guess its not important, but that I don't know.  ("One right way of doing things" my expletive)

Comment 4

[Jeff Hammel]

Comment on attachment 713542 [details] [diff] [review]
Patch 1.0 - handle prefs properly

it would be nice to have a failing test that is fixed by this

Comment 5

[Andrew Halberstadt [:ahal]]

https://github.com/mozilla/mozbase/commit/4f7436efe1c50084caf3bb506e584f015817c6cf

I changed import cStringIO to from StringIO import StringIO and also added a test that fails without this patch and passes with it.