Andrew Halberstadt [:ahal] Assignee
	Description • 11 years ago

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

	Jeff Hammel
	Comment 1 • 11 years ago

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 :/

	Andrew Halberstadt [:ahal] Assignee
	Comment 2 • 11 years ago

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.

	Jeff Hammel
	Comment 3 • 11 years ago

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)

	Jeff Hammel
	Comment 4 • 11 years ago

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

	Andrew Halberstadt [:ahal] Assignee
	Comment 5 • 11 years ago

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.