Edgewall Software
Modify

Opened 18 years ago

Closed 18 years ago

Last modified 18 years ago

#3425 closed enhancement (fixed)

WebAdmin should be more conservative with trac.ini

Reported by: Emmanuel Blot Owned by: Christian Boos
Priority: normal Milestone: 0.10
Component: admin/web Version: devel
Severity: minor Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

WebAdmin changes the contents of the trac.ini config file to reflect the configuration changes made by the user.

However, it does not preserve the original line ordering (nor the section ordering).

This makes the file hard to read for sections with a lot of options, especially with options which are tied to the same feature (e.g. login and password) and it makes the comparison of config files of different projects hosted on the same server a nightmare.

WebAdmin should ideally preserve the original ordering, or at least sort the options in lexical order.

Attachments (2)

orderedconfig.patch (6.0 KB ) - added by Emmanuel Blot 18 years ago.
Ordered config parser (w/o the odict.py), for [3542]
config_save_in_lexical_ordering-r3538.patch (1.4 KB ) - added by Christian Boos 18 years ago.
Use simply the lexical ordering of sections and options within each sections, instead of trying to preserve the original ordering

Download all attachments as: .zip

Change History (19)

comment:1 by Matthew Good, 18 years ago

The writing behavior is how the Python ConfigParser module operates. I don't know if there are any open requests to fix this in the Python library, but I suppose it wouldn't be available soon.

ConfigObj supports a super-set of the ini-style format currently used, though I don't know if it has any better saving behavior.

comment:2 by Emmanuel Blot, 18 years ago

I was thinking about augmenting the ConfigParser class, so that it preserves the original order of trac.ini file

For example, the ordered dictionary (BSD license) could be used to store the sections and options rather than in the default Python dictionary used by the ConfigParser.

I'm attaching a simple proof-of-concept patch for the current trunk. I made a couple of tests with WebAdmin and the trac.ini looked ok. I also ran the non-regression test suite. Basically, the patch:

  • adds the odict.py module
  • overrides the __init__, add_section and _read methods of the ConfigParser
  • replaces the ConfigParser used in the config.py file with an OrderedConfigParser
  • changes the way the trac.ini file is built (filter out options from the existing parser rather than building a new one)

I did not find another way but overrides the _readmethod of the ConfigParser for a single-line change ;-(

The patch is a bit overkill for a "simple" add-on like this on.

by Emmanuel Blot, 18 years ago

Attachment: orderedconfig.patch added

Ordered config parser (w/o the odict.py), for [3542]

comment:3 by Emmanuel Blot, 18 years ago

BTW, the odict.py file can probably skimmed off, to keep the relevant methods used by ConfigParser and discard the useless ones.

comment:4 by Christian Boos, 18 years ago

Maybe the approach is naive, but wouldn't it be simpler to just write our own "save" method? We sort the config sections and entries and write a few key = value lines, and voila ;)

Is there something I don't see?

comment:5 by Emmanuel Blot, 18 years ago

(rah, the reply feature is really in trouble w/ Safari). Anyway:

wouldn't it be simpler to just write our own "save" method?

A custom 'save' method would allow to define the order of the lines in the config file, but you first need to know the original order: the user may reorganise the file for his needs, there are several plugin that are now using the configuration file and a "save" method would definitely change the order of the plugin sections - which makes the comparison of projects more difficult.

Or, I missing an important point. Anyway, if there is a simpler method, I'm +1.

comment:6 by Christian Boos, 18 years ago

WebAdmin should ideally preserve the original ordering, or at least sort the options in lexical order.

I was targeting the "at least" part and you the "ideally" part ;)

I think that if the order remains consistent, that's good enough. If we go the "ideal" way, then I would really like that we also preserve comments.

comment:7 by Christian Boos, 18 years ago

Milestone: 0.10
Owner: changed from Christopher Lenz to Christian Boos

To illustrate what I was saying above: attachment:config_save_in_lexical_ordering-r3538.patch

This makes it easy to compare trac.ini files saved that way. Note also that new configuration files will be created that way too, so the loss of the "original" order will only affect existing installations.

by Christian Boos, 18 years ago

Use simply the lexical ordering of sections and options within each sections, instead of trying to preserve the original ordering

comment:8 by Christian Boos, 18 years ago

Keywords: review added

comment:9 by Emmanuel Blot, 18 years ago

It seems ok, although it makes me think about a related question:
How do the configuration code cope with unicode values (for characters not in ASCII set)?

It seems the ConfigParser does not make any character set translation at all. So when we read UTF-8 from the ini file, the configuration parser uses utf-8 values. What about if we place unicode characters in the configuration parser and call the save() method? It seems that neither the original parser method or the patch method can cope with Unicode. Am I wrong?

I don't see where unicode-to-utf8 translation is made for the configuration file.

BTW: it there any reason why the 'top level' scripts in /scripts directory are kept encoded using iso-8859-1 rather than utf-8 as the rest of the code? ex:

file -i scripts/trac-admin
scripts/trac-admin: text/plain; charset=iso-8859-1

in reply to:  9 ; comment:10 by anonymous, 18 years ago

Keywords: review removed
Owner: changed from Christian Boos to anonymous
Priority: lowestnormal
Status: newassigned

Replying to eblot:

It seems the ConfigParser does not make any character set translation at all. So when we read UTF-8 from the ini file, the configuration parser uses utf-8 values. What about if we place unicode characters in the configuration parser and call the save() method? It seems that neither the original parser method or the patch method can cope with Unicode. Am I wrong?

Partly: it works fine with Unicode values, but the current way to do the save() is not optimal: I first did the correct thing to do, i.e. to_unicode(value).encode('utf-8'), but then overwrote the patch with a simpler one, which just used value. That second solution ensured that the original charset used for the trac.ini file would be preserved. But OTOH, when a config value is programmatically changed (as with WebAdmin), the encoding used will be "utf-8" (see Section.set()) and therefore we would eventually end with a mix of charsets, which is not good ;)

So I think it's not that important if the original charset is lost, and "utf-8" is used instead. I'll add a # -*- coding: utf-8 -*- first line to the file, to make that clear.

Also, there's no support yet for Unicode keys and section names, as I think this would have been overkill.

BTW: it there any reason why the 'top level' scripts in /scripts directory are kept encoded using iso-8859-1 rather than utf-8 as the rest of the code? ex:

Oversight, probably…

in reply to:  10 ; comment:11 by Emmanuel Blot, 18 years ago

Owner: changed from anonymous to Christian Boos
Status: assignednew

Replying to anonymous:

Is anonymous eq cboos ?

Partly: it works fine with Unicode values,

but "%s" % (unicode_value) generates a UnicodeEncodeError, doesn't it?

the encoding used will be "utf-8"

I fully agree.

I'll add a # -*- coding: utf-8 -*- first line to the file, to make that clear.

You mean in the trac.ini file? Make it less pythonic (users/admin don't have to care about the language and naming conventions used for Trac), something like

; charset: UTF-8

Also, there's no support yet for Unicode keys and section names, as I think this would have been overkill.

Plugins may need non-ascii values. A typical case is the th:TracFrenchTranslation which stores the translated term in the ini file. The configuration file should support UTF-8 natively (but no other encoding scheme).

in reply to:  11 comment:12 by Christian Boos, 18 years ago

Resolution: fixed
Status: newclosed

Sorry, I applied attachment:config_save_in_lexical_ordering-r3538.patch (improved the way described in comment:10) as r3556, before seeing your reply…

Replying to eblot:

Replying to anonymous: Is anonymous eq cboos ?

Sure ;)

but "%s" % (unicode_value) generates a UnicodeEncodeError, doesn't it?

no, it gives you back unicode_value (i.e. is equivalent to u"%s")

I'll add a # -*- coding: utf-8 -*- first line to the file, to make that clear.

You mean in the trac.ini file? Make it less pythonic (users/admin don't have to care about the language and naming conventions used for Trac), something like

It's not (only) a Trac or Python convention, it's an Emacs convention and I believe Emacs is widely used.

Also, there's no support yet for Unicode keys and section names, as I think this would have been overkill.

Plugins may need non-ascii values. A typical case is the th:TracFrenchTranslation which stores the translated term in the ini file. The configuration file should support UTF-8 natively (but no other encoding scheme).

Ok, I'll think about adding this later.

comment:13 by Emmanuel Blot, 18 years ago

It's not (only) a Trac or Python convention, it's an Emacs convention and I believe Emacs is widely used.

Administrators from the Windows world are probably using other kinds of editors. I work/develop on Win/Linux/Mac, I never used it. Btw, ';' is more common in ini files than '#'. Not a big deal though ;-)

comment:14 by Emmanuel Blot, 18 years ago

Resolution: fixed
Status: closedreopened

The unit test fails on Mac OS X (10.4.7/Intel), with both Python 2.3.5 and Python 2.4.3. It fails for Latin-1 strings:

amazas:~/Sources/Svn/edgewall.com/trunk eblot$ python trac/tests/config.py
............F
======================================================================
FAIL: test_set_and_save (__main__.ConfigurationTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac/tests/config.py", line 123, in test_set_and_save
    self.assertEquals(u"Voilà l'été", config.get('a', 'option3'))
AssertionError: u"Voil\xe0 l'\xe9t\xe9" != u"Voil\u2021 l'\xc8t\xc8"

----------------------------------------------------------------------
Ran 13 tests in 1.022s

FAILED (failures=1)

Do we need to support Latin-1 encoding?

comment:15 by Christian Boos, 18 years ago

Resolution: fixed
Status: reopenedclosed

Ok, that's fixed now in r3560.

comment:16 by nanotube, 18 years ago

hi i was recently looking to solve the same problem for my own development, and i came across "configobj", which is basically like configparser, but better in many respects, including the preservation of original file structure, and even comments. check it out, here: http://www.voidspace.org.uk/python/configobj.html

so you may want to try using it instead of the default configparser module, so that you can build on the shoulders of others, rather than hacking together your own solution. :)

in reply to:  16 comment:17 by anonymous, 18 years ago

Replying to nanotube:

hi i was recently looking to solve the same problem for my own development, and i came across "configobj", which is basically like configparser, but better in many respects, including the preservation of original file structure, and even comments. check it out, here: http://www.voidspace.org.uk/python/configobj.html

so you may want to try using it instead of the default configparser module, so that you can build on the shoulders of others, rather than hacking together your own solution. :)

ConfigObj saves in order and has full unicode support.

Fuzzyman http://www.voidspace.org.uk/python/configobj.html

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.