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

config: make type conversion explicit #357

Merged
merged 2 commits into from
Apr 15, 2014
Merged

Conversation

carlosmn
Copy link
Member

The type of a config value depends on the tool that interprets
it. Parsing eagerly can lead to a situation where we return a bool
instead of a string or a number.

Let the user specify the type themselves by passing in a (str, type)
tuple into the mapping interface.

@carlosmn
Copy link
Member Author

Well, it seems on python 2 I can only use long, but in python 3, I can only use int. Is there something short of a string that I can use here?

@carlosmn
Copy link
Member Author

Looks like int should actually work everywhere, but I'm using a bad check.

The type of a config value depends on the tool that interprets
it. Parsing eagerly can lead to a situation where we return a bool
instead of a string or a number.

Let the user specify the type themselves by passing in a (str, type)
tuple into the mapping interface.
@jdavid
Copy link
Member

jdavid commented Mar 29, 2014

The API troubles me. The mapping interface is key: value, but the type is not part of the key. This should be implemented as a method.

@carlosmn
Copy link
Member Author

I was trying to stay within the mapping interface, but you're right that it's mixing two different pieces of information.

Passing a tuple to the mapping interface isn't the best of interfaces,
as the key is only the string.

Instead, expose `Config.get_bool()` and `Config.get_int()` methods to
parse the values as per the git-config rules before returning the
appropriate type to the user.

The mapping interface itself returns a string.
@carlosmn
Copy link
Member Author

Changed API to have get_bool() and get_int(). The mapping interface returns a string.

Would you like me to squash this into one?

@jdavid jdavid merged commit 73e9e58 into libgit2:master Apr 15, 2014
@jdavid
Copy link
Member

jdavid commented Apr 15, 2014

If anything I would remove the mapping interface and implement get_str() instead, to make the API consistent.

By the way, I get an error building the documentation:

$ make html
    ...
    .../pygit2/docs/config.rst:27: ERROR: Unknown interpreted text role "method".
    .../pygit2/docs/config.rst:27: ERROR: Unknown interpreted text role "method".
    ...

@carlosmn
Copy link
Member Author

This is what happens when you assume instead of setting up paths correctly. For whatever reason the keyword for method is meth instead of method. I'll push up a fix.

@carlosmn
Copy link
Member Author

Re dropping mapping: asking for a single value as a string is a very common operation, which corresponds quite well to dictionary access (even if there's some funky logic as to how the key gets hashed), so I'd like to keep it.

The other operations exist mostly because the config format has its own rules as to what constitutes a boolean value and integers have their own suffix rules.

We could also go the route of providing static functions that expose the parsing, which would get rid of multiple ways of accessing the value itself but still let the user parse with git-config rules, something like

    Config.parse_int(config['core.deltabasecachelimit'])

This would also let the user apply these rules to data that doesn't come directly from the config but which has the same semantics. Come to think of it, we should expose this anyway and then we can think about whether get_{int,bool} should be removed.

@jdavid
Copy link
Member

jdavid commented Apr 18, 2014

Okey.

By the way, I guess the config stuff is also a good candidate for cffi.

@carlosmn
Copy link
Member Author

Yeah, that's what I was thinking as well. There's a bit of bootstrapping that it needs, but once the remote-cffi PR is merged, I can start working on that.

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