[PATCH RFC] scmutil: add a simple key-value file helper

Kostia Balytskyi kobalyts at outlook.com
Mon Dec 5 17:49:38 EST 2016


On 12/5/16 9:54 PM, Jun Wu wrote:
> Excerpts from Kostia Balytskyi's message of 2016-12-03 15:37:55 -0800:
>> # HG changeset patch
>> # User Kostia Balytskyi <ikostia at fb.com>
>> # Date 1480808065 28800
>> #      Sat Dec 03 15:34:25 2016 -0800
>> # Node ID c4312906ef7d06dc3be1575ee5cde574b915c9c4
>> # Parent  715232cf9182f904d5153862a1ec4905faaeee6e
>> scmutil: add a simple key-value file helper
>>
>> The purpose of the added class is to serve purposes like save files of shelve
>> or state files of shelve, rebase and histedit. Keys of these files can be
>> alphanumeric and start with letters, while values must not contain newlines.
>> Keys which start with an uppercase letter are required, while other keys
>> are optional.
>>
>> In light of Mercurial's reluctancy to use Python's json module, this tries
>> to provide a reasonable alternative for a non-nested named data.
>> Comparing to current approach of storing state in plain text files, where
>> semantic meaning of lines of text is only determined by their oreder,
>> simple key-value file allows for reordering lines and thus helps handle
>> optional values.
>>
>> Initial use-case I see for this is obs-shelve's shelve files. Later we
>> can possibly migrate state files to this approach.
>>
>> The test is in a new file beause I did not figure out where to put it
>> within existing test suite. If you give me a better idea, I will gladly
>> follow it.
>>
>> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
>> --- a/mercurial/scmutil.py
>> +++ b/mercurial/scmutil.py
>> @@ -1571,3 +1571,51 @@ class checkambigatclosing(closewrapbase)
>>       def close(self):
>>           self._origfh.close()
>>           self._checkambig()
>> +
>> +class simplekeyvaluefile(object):
>> +    """A simple file with key=value lines
>> +
>> +    Kyes must be alphanumerics and start with a letter, values might not
> Keys.
Good catch.
>
>> +    contain '\n' characters
>> +    """
>> +    class InvalidKeyInFileException(Exception): pass
>> +    class InvalidValueInFileException(Exception): pass
>> +    class MissingRequiredKeyInFileException(Exception): pass
> This enforces the caller to do
>
>    excpet simplekeyvaluefile.InvalidKeyInFileException
>
> I guess we put every exception classes inside error.py?
> (See below, I think the exceptions used in "write" should be just
>   "RuntimeError"s)
How strong is our habit of storing exceptions in error.py? I feel like
having exceptions namespaced in simplekeyvaluefile reduces the need
to have really verbose names. In short, I am for keeping exceptions in
the class, but I don't feel too strongly about it.
>
>> +
>> +    # if KEYS is non-empty, read values are validated against it:
>> +    # - if field name starts with an uppercase letter, this FIELDS
>> +    #   is considered required and its absense is an Exception
>> +    # - if field name starts with a lowercase letter, it is optional
>> +    #   and serves mainly as a reference for code reader
> This may make the file look a bit strange when editing directly (mixed of
> upper case and lower cases). As the coding style is basically "lowercase
> everywhere". I'd suggest underscore or an explicit boolean flag instead.
Will do.
>
>> +    KEYS = []
>> +
>> +    def __init__(self, repo, path):
>> +        self.vfs = repo.vfs
>> +        self.path = path
>> +
>> +    def validate(self, d):
>> +        for k in self.KEYS:
>> +            if k[0].isupper() and k not in d:
>> +                e = _("Missing a required key: '%s'" % k)
> I think we use lower-case in exception message ("missing" vs "Missing").
Will fix.
>
>> +                raise self.MissingRequiredKeyInFileException(e)
>> +
>> +    def read(self):
>> +        lines = self.vfs.readlines(self.path)
>> +        d = dict(line[:-1].split('=', 1) for line in lines)
>> +        self.validate(d)
>> +        return d
>> +
>> +    def write(self, data):
> data deserves a docstring about its type.
Good idea, will fix.
>
>> +        lines = []
>> +        for k, v in data.items():
>> +            if not k[0].isalpha():
>> +                e = _("Keys must start with a letter in a key-value file")
>> +                raise self.InvalidKeyInFileException(e)
>> +            if not k.isalnum():
>> +                e = _("Invalid key name in a simple key-value file")
>> +                raise self.InvalidKeyInFileException(e)
>> +            if '\n' in v:
>> +                e = _("Invalid value in a simple key-value file")
>> +                raise self.InvalidValueInFileException(e)
> These sound like a programmer error. Maybe just raise RuntimeError.
I agree with Augie and I see no harm in having an extra exception that 
will basically only fire during development phases.
>
>> +            lines.append("%s=%s\n" % (k, v))
>> +        self.vfs.writelines(self.path, lines)
> Probably use atomictemp to avoid race conditions.
>
>> diff --git a/tests/test-other.t b/tests/test-other.t
>> new file mode 100644
>> --- /dev/null
>> +++ b/tests/test-other.t
>> @@ -0,0 +1,95 @@
>> +Test simple key-value files
>> +  $ cd $TESTTMP
>> +  $ hg init repo
>> +  $ cd $TESTTMP/repo
>> +
>> +Test simple key-value file creation
>> +  $ cat <<EOF > keyvalwriter.py
>> +  > from mercurial import ui, hg
>> +  > from mercurial.scmutil import simplekeyvaluefile
>> +  > ui = ui.ui()
>> +  > repo = hg.repository(ui, '$TESTTMP/repo')
>> +  > d = {'key1': 'value1', 'Key2': 'value2'}
>> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
>> +  > EOF
>> +  $ python keyvalwriter.py
>> +  $ cat .hg/kvfile | sort
>> +  Key2=value2
>> +  key1=value1
>> +
>> +Test simple key-value file reading with invalid keys or values
>> +  $ cat <<EOF > keyvalwriter.py
>> +  > from mercurial import ui, hg
>> +  > from mercurial.scmutil import simplekeyvaluefile
>> +  > ui = ui.ui()
>> +  > repo = hg.repository(ui, '$TESTTMP/repo')
>> +  > d = {'0key1': 'value1', 'Key2': 'value2'}
>> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
>> +  > EOF
>> +  $ python keyvalwriter.py 2>&1 | tail -1
>> +  mercurial.scmutil.InvalidKeyInFileException: Keys must start with a letter in a key-value file
>> +  $ cat <<EOF > keyvalwriter.py
>> +  > from mercurial import ui, hg
>> +  > from mercurial.scmutil import simplekeyvaluefile
>> +  > ui = ui.ui()
>> +  > repo = hg.repository(ui, '$TESTTMP/repo')
>> +  > d = {'key at 1': 'value1'}
>> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
>> +  > EOF
>> +  $ python keyvalwriter.py 2>&1 | tail -1
>> +  mercurial.scmutil.InvalidKeyInFileException: Invalid key name in a simple key-value file
>> +  $ cat <<EOF > keyvalwriter.py
>> +  > from mercurial import ui, hg
>> +  > from mercurial.scmutil import simplekeyvaluefile
>> +  > ui = ui.ui()
>> +  > repo = hg.repository(ui, '$TESTTMP/repo')
>> +  > d = {'key1': 'value\n1'}
>> +  > kvf = simplekeyvaluefile(repo, 'kvfile').write(d)
>> +  > EOF
>> +  $ python keyvalwriter.py 2>&1 | tail -1
>> +  mercurial.scmutil.InvalidValueInFileException: Invalid value in a simple key-value file
>> +
>> +Test simple key-value file reading without field list
>> +  $ cat <<EOF > keyvalreader.py
>> +  > from mercurial import ui, hg
>> +  > from mercurial.scmutil import simplekeyvaluefile
>> +  > ui = ui.ui()
>> +  > repo = hg.repository(ui, '$TESTTMP/repo')
>> +  > d = simplekeyvaluefile(repo, 'kvfile').read()
>> +  > for k, v in sorted(d.items()):
>> +  >     print "%s => %s" % (k, v)
>> +  > EOF
>> +  $ python keyvalreader.py
>> +  Key2 => value2
>> +  key1 => value1
>> +
>> +Test simeple key-value file when necessary files are present
>> +  $ cat <<EOF > keyvalreader.py
>> +  > from mercurial import ui, hg
>> +  > from mercurial.scmutil import simplekeyvaluefile
>> +  > ui = ui.ui()
>> +  > repo = hg.repository(ui, '$TESTTMP/repo')
>> +  > class kvf(simplekeyvaluefile):
>> +  >     KEYS = ['key3', 'Key2']
>> +  > d = kvf(repo, 'kvfile').read()
>> +  > for k, v in sorted(d.items()):
>> +  >     print "%s => %s" % (k, v)
>> +  > EOF
>> +  $ python keyvalreader.py
>> +  Key2 => value2
>> +  key1 => value1
>> +
>> +Test simeple key-value file when necessary files are absent
>> +  $ cat <<EOF > keyvalreader.py
>> +  > from mercurial import ui, hg
>> +  > from mercurial.scmutil import simplekeyvaluefile
>> +  > ui = ui.ui()
>> +  > repo = hg.repository(ui, '$TESTTMP/repo')
>> +  > class kvf(simplekeyvaluefile):
>> +  >     KEYS = ['Key4', 'Key2']
>> +  > d = kvf(repo, 'kvfile').read()
>> +  > for k, v in sorted(d.items()):
>> +  >     print "%s => %s" % (k, v)
>> +  > EOF
>> +  $ python keyvalreader.py 2>&1 | tail -1
>> +  mercurial.scmutil.MissingRequiredKeyInFileException: Missing a required key: 'Key4'
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel



More information about the Mercurial-devel mailing list