Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comments assigned to wrong sections #48

Closed
ntrrgc opened this issue Apr 5, 2014 · 10 comments
Closed

Comments assigned to wrong sections #48

ntrrgc opened this issue Apr 5, 2014 · 10 comments

Comments

@ntrrgc
Copy link

ntrrgc commented Apr 5, 2014

This is an over-simplified version of zypper.conf:

## Configuration file for Zypper.

[main]

## Show repository alias instead of name.
# showAlias = false

## Columns to show in repository list printed by repos (lr) command by default.
# repoListColumns = Anr

[solver]

## Do not install soft dependencies (recommended packages)
# installRecommends = yes

[color]

## Whether to use colors
# useColors = never

When read with configobj, comments are assigned to sections in an awkward fashion:

>>> config = configobj.ConfigObj('zypper.conf')
>>> config.comments
{'color': ['',
  '## Do not install soft dependencies (recommended packages)',
  '# installRecommends = yes',
  ''],
 'main': [],
 'solver': ['',
  '## Show repository alias instead of name.',
  '# showAlias = false',
  '',
  '## Columns to show in repository list printed by repos (lr) command by default.',
  '# repoListColumns = Anr',
  '']}

As a consequence, if I run del config['solver'] I am deleting comments of main and preserving comments of solver, leading to a very confusing configuration file:

--- zypper.before.conf   2014-04-06 00:20:16.432652464 +0200
+++ zypper.after.conf    2014-04-06 00:26:16.569526812 +0200
@@ -2,14 +2,6 @@

 [main]

-## Show repository alias instead of name.
-# showAlias = false
-
-## Columns to show in repository list printed by repos (lr) command by default.
-# repoListColumns = Anr
-
-[solver]
-
 ## Do not install soft dependencies (recommended packages)
 # installRecommends = yes
@robdennis
Copy link
Member

this is a great report in that it includes both a configuration and an
explanation of what your expected values are. The next step is to confirm
the behavior on 4.7.2, and I'll take a shot at that hopefully by tomorrow.

If you're able to do that before I am, and can post the results, that can
get me started on fixing it a bit quicker, but certainly not required.
Thanks

On Sat, Apr 5, 2014 at 6:29 PM, ntrrgc notifications@github.com wrote:

This is an over-simplified version of zypper.conf:

Configuration file for Zypper.

[main]

Show repository alias instead of name.

showAlias = false

Columns to show in repository list printed by repos (lr) command by default.

repoListColumns = Anr

[solver]

Do not install soft dependencies (recommended packages)

installRecommends = yes

[color]

Whether to use colors

useColors = never

When read with configobj, comments are assigned to sections in an awkward
fashion:

config = configobj.ConfigObj('zypper.conf')
config.comments
{'color': ['',
'## Do not install soft dependencies (recommended packages)',
'# installRecommends = yes',
''],
'main': [],
'solver': ['',
'## Show repository alias instead of name.',
'# showAlias = false',
'',
'## Columns to show in repository list printed by repos (lr) command by default.',
'# repoListColumns = Anr',
'']}

As a consequence, if I run del config['solver'] I am deleting comments of
main and preserving comments of solver, leading to a very confusing
configuration file:

--- zypper.before.conf 2014-04-06 00:20:16.432652464 +0200
+++ zypper.after.conf 2014-04-06 00:26:16.569526812 +0200
@@ -2,14 +2,6 @@

[main]

-## Show repository alias instead of name.

-# showAlias = false

-## Columns to show in repository list printed by repos (lr) command by default.

-# repoListColumns = Anr

-[solver]

Do not install soft dependencies (recommended packages)

installRecommends = yes

Reply to this email directly or view it on GitHubhttps://github.com//issues/48
.

@ntrrgc
Copy link
Author

ntrrgc commented Apr 6, 2014

It happens the same in 4.7.2.

@robdennis
Copy link
Member

Ah, great. Based on your description this does sound like a bug, and the fact it happens on 4.7.2 means that I don't need to track down what we changed when we made the python 3 changes.

I'd like to take a look at this this afternoon

On Sun, Apr 6, 2014 at 3:45 AM, ntrrgc notifications@github.com wrote:

It happens the same in 4.7.2.

Reply to this email directly or view it on GitHub:
#48 (comment)

@robdennis robdennis added this to the 5.0.4 - next bugfix release milestone Apr 6, 2014
@robdennis robdennis added the bug label Apr 6, 2014
@robdennis robdennis self-assigned this Apr 7, 2014
@robdennis
Copy link
Member

@ntrrgc, I took a look at this, and I believe the confusion stems from configobj assumes that section comments are "above" the sections, and when all values within a section are commented out, that essentially makes all the comments "under" [main] be interpreted as "over" [solver]

I feel like you're expecting that comments are associated with the section that they are "under"

here's the original results you reported:

>>> pprint.pprint(configobj.ConfigObj('zypper.cfg').comments)
{'color': ['',
           '## Do not install soft dependencies (recommended packages)',
           '# installRecommends = yes',
           ''],
 'main': [],
 'solver': ['',
            '## Show repository alias instead of name.',
            '# showAlias = false',
            '',
            '## Columns to show in repository list printed by repos (lr) command by default.',
            '# repoListColumns = Anr',
            '']}

The file also yields these two things that you may not have known about (at least, you didn't post it)

>>> commented_zypper.initial_comment
['## Configuration file for Zypper.', '']
>>> commented_zypper.final_comment
['', '## Whether to use colors', '# useColors = never']

if I uncomment the things that you commented out:

## Configuration file for Zypper.

[main]

## Show repository alias instead of name.
showAlias = false

## Columns to show in repository list printed by repos (lr) command by default.
repoListColumns = Anr

[solver]

## Do not install soft dependencies (recommended packages)
installRecommends = yes

[color]

## Whether to use colors
useColors = never

here's the new results:

>>> pprint.pprint(configobj.ConfigObj('zypper_uncommented.cfg').comments)
{'color': [''], 'main': [], 'solver': ['']}

Well, this is unexpected, but this is because none of these are considered section comments now. Here's how it breaks down:

>>> uncommented_zypper.initial_comment
['## Configuration file for Zypper.', '']
>>> uncommented_zypper.final_comment
[]
>>> uncommented_zypper['color'].comments
{'useColors': ['', '## Whether to use colors']}
>>> uncommented_zypper['main'].comments
{'showAlias': ['', '## Show repository alias instead of name.'], 'repoListColumns': ['', '## Columns to show in repository list printed by repos (lr) command by default.']}
>>> uncommented_zypper['solver'].comments
{'installRecommends': ['', '## Do not install soft dependencies (recommended packages)']}

As long as you have a key/value (possibly a subsection?) in a section, that will associate the comments "under" a section with the key, and not with the following section.

You said that this was a simplified version of your config, is the "full-complexity" config worth posting with updated expected results?

Or, with this clarification in mind, perhaps this is really a documentation ticket, and you'd either need to:

  • comment out the section headings that have 100% commented out content (since they aren't specifying anything anyway)
  • provide a dummy key-value pair to make sure that your existing sections have at least something there

I don't think the "over" vs "under" for section comments is something that we can change up due to backwards compatibility concerns, so I'm hoping that the explanation here leads us to a good spot.

@robdennis
Copy link
Member

removed bug and added documentation label, this ticket will be held open until @ntrrgc can weigh in on a doc change / tweaking the config based on this explanation works

@ntrrgc
Copy link
Author

ntrrgc commented Apr 7, 2014

Well, indeed I knew about initial_comment and final_comment, but I wonder why comments are picked below rather than above. I don't recall ever seeing a configuration file which places comments below.

The full configuration file (zypper.conf) is fully commented out by default.

In case you're wondering, I use this module here. The purpose is to tweak settings in ini-style files programatically without messing with its format. Although deleting sections is not a commonly performed operation, I thought the behavior is strange enough to post it here.

@robdennis
Copy link
Member

If I had to guess, it's this way because "key comments" make sense above the key and going above for everything was simpler than applying a heuristic to differentiate between section and key comments (e.g. Break on multiple new lines).

I could see this being something that is revisited in the event we start entertaining backwards incompatible changes. I don't have a good sense of what a good comment breakdown looks like, so if you have any suggestions I'd love to hear it.

On Mon, Apr 7, 2014 at 2:25 AM, ntrrgc notifications@github.com wrote:

Well, indeed I knew about initial_comment and final_comment, but I wonder why comments are picked below rather than above. I don't recall ever seeing a configuration file which places comments below.
The full configuration file (zypper.conf) is fully commented out by default.

In case you're wondering, I use this module here. The purpose is to tweak settings in ini-style files programatically without messing with its format. Although deleting sections is not a commonly performed operation, I thought the behavior is strange enough to post it here.

Reply to this email directly or view it on GitHub:
#48 (comment)

@ntrrgc
Copy link
Author

ntrrgc commented Apr 7, 2014

Maybe stopping on lines than contain an equal sign?

@robdennis
Copy link
Member

interesting thought. Something to consider in the future

@frakman1
Copy link

frakman1 commented Dec 8, 2018

On the topic of comments in a config file.
Does this library allow you to insert comments programmatically?
I want to be able to initialize a config file to a default (at init and when errors are found) and include comments above the various sections. I don't have a good way to do this without manually manipulating the file after setting it up using the config obj library.
Is there a better way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants