Skip to content

Update Config.__iter__ to match the documentation #776

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

Conversation

thecjharries
Copy link
Contributor

Problem

The Config.__iter__ docs say the method iterates over all entries, where "[e]ach element is a tuple containing the name and the value", which could be problematic as the method "may return multiple versions of each entry if they are set multiple times." However, the (final) next method on the iterator only returns the name of the entry. There's a very good chance it's never returned a tuple, evidenced by the blame.

Example

from __future__ import print_function
from tempfile import NamedTemporaryFile
from pygit2 import Config

# Start with a blank slate
blank = Config()

# https://git-scm.com/docs/git-config#git-config-pushdefault
for index, value in list(enumerate(['nothing', 'current', 'upstream'])):
    # There's probably a better way to do this
    with NamedTemporaryFile() as config_file:
        config_file.write("[push]\n\tdefault = %s" % value)
        config_file.seek(0)
        blank.add_file(config_file.name, index)

# This executes Config.__iter__
for key in blank:
    # Since it doesn't return a value, we're stuck with the default return
    print(key, blank[key])
# push.default upstream
# push.default upstream
# push.default upstream

# It's not actually a multivar, but we can see everything this way
for value in blank.get_multivar('push.default'):
    print(value)
# nothing
# current
# upstream

Pull Request

I updated __next__'s return to also include the value. This matches what the docs describe, returning a tuple of name and value. Since the change only affects the iterator, the default value return isn't affected.

I also updated the test to consume the tuple. There's probably more that could go there, but I'm not quite sure what yet.

libgit2

I think libgit2 supports my interpretation of things. git_config_iterator iterates over git_config_entry, which contains both the name and the value (as well as some other information that might be useful to use).

Extension

Looking at my example above, it doesn't seem like (key, value) is all that useful. It might be a good idea to also return level. It's in git_config_entry, so it's available to the iterator.

Alternative

As I pointed out, I don't think this has worked as intended for years. An equally simple solution would be to update the docs to match the current behavior. I'd be more than happy to do that if this PR is rejected.

@jdavid
Copy link
Member

jdavid commented Mar 15, 2018

Good, thanks. Since we're at this better add the level now. Maybe the iterator should return ConfigEntry objects.

@thecjharries
Copy link
Contributor Author

I can do that. Once I get off work and get home, I'll knock that out. Should I update this PR or make a new one? I don't mind either way.

@jdavid
Copy link
Member

jdavid commented Mar 16, 2018

as you prefer

@thecjharries
Copy link
Contributor Author

This PR is superseded by #778 and can be closed without merging.

@thecjharries thecjharries deleted the feature/update-config-iter-to-match-docs branch March 17, 2018 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants