# HG changeset patch # User Steve Losh # Date 1276983605 14400 # Node ID dff2e396a30f1508a0029d355fbaa4b980830f97 # Parent 03fded64e34ee471f36f48e958e83b07312c3cc6 cli: add support for editing comments diff -r 03fded64e34e -r dff2e396a30f review/cli.py --- a/review/cli.py Sat Jun 19 17:39:57 2010 -0400 +++ b/review/cli.py Sat Jun 19 17:40:05 2010 -0400 @@ -11,18 +11,58 @@ from mercurial.node import short +def _get_datastore(ui, repo): + try: + return api.ReviewDatastore(ui, repo) + except api.UninitializedDatastore: + raise util.Abort(messages.NO_DATA_STORE) + def _get_message(ui, rd, initial): message = ui.edit(initial, rd.repo.ui.username()) return '\n'.join(l for l in message.splitlines() if not l.startswith('HG: ')).strip() +def _edit_comment(rd, comment, *fnames, **opts): + lines = opts.pop('lines') + message = opts.pop('message').strip() + mdown = opts.pop('mdown') + + if len(fnames) > 1: + raise util.Abort(messages.EDIT_REQUIRES_ONE_OR_LESS_FILES) + + if not lines: + lines = comment.lines + else: + lines = [int(l.strip()) for l in lines.split(',')] + + if not message: + message = comment.message + + if not fnames: + fnames = [comment.filename] + else: + fnames = [api.sanitize_path(fnames[0])] + + style = 'markdown' if mdown or comment.style == 'markdown' else '' + + try: + rd.edit_comment(comment.identifier, message, fnames[0], lines, style) + except api.FileNotInChangeset: + raise util.Abort(messages.COMMENT_FILE_DOES_NOT_EXIST % ( + fnames[0], rd.target[comment.node].rev())) + +def _edit_signoff(rd, signoff, **opts): + raise util.Abort('unimplemented, sorry') + + def _web_command(ui, repo, **opts): ui.note(messages.WEB_START) read_only = opts.pop('read_only') allow_anon = opts.pop('allow_anon') address = opts.pop('address') port = int(opts.pop('port')) + rd = _get_datastore(ui, repo) import web web.load_interface(ui, repo, read_only=read_only, allow_anon=allow_anon, @@ -52,8 +92,8 @@ lines = opts.pop('lines') message = opts.pop('message').strip() mdown = opts.pop('mdown') + rd = _get_datastore(ui, repo) - rd = api.ReviewDatastore(ui, repo) rcset = rd[rev] if lines and not len(fnames) == 1: @@ -80,11 +120,11 @@ fn, repo[rev].rev())) def _signoff_command(ui, repo, **opts): - rd = api.ReviewDatastore(ui, repo) - rcset = rd[opts.pop('rev')] message = opts.pop('message').strip() force = opts.pop('force') mdown = opts.pop('mdown') + rd = _get_datastore(ui, repo) + rcset = rd[opts.pop('rev')] yes, no = opts.pop('yes'), opts.pop('no') if yes and no: @@ -105,7 +145,7 @@ rcset.add_signoff(message=message, opinion=opinion, force=force, style=style) def _check_command(ui, repo, **opts): - rd = api.ReviewDatastore(ui, repo) + rd = _get_datastore(ui, repo) rcset = rd[opts.pop('rev')] if opts.pop('no_nos'): @@ -127,11 +167,8 @@ def _review_command(ui, repo, *fnames, **opts): rev = opts.pop('rev') context = int(opts.pop('unified')) + rd = _get_datastore(ui, repo) - try: - rd = api.ReviewDatastore(ui, repo) - except api.UninitializedDatastore: - raise util.Abort(messages.NO_DATA_STORE) cset = repo[rev] rcset = rd[rev] @@ -242,22 +279,37 @@ def _delete_command(ui, repo, *identifiers, **opts): # TODO: require -f to delete some else's item force = opts.pop('force') + rd = _get_datastore(ui, repo) if not identifiers: - raise util.Abort(messages.DELETE_REQUIRES_IDS) - - try: - rd = api.ReviewDatastore(ui, repo) - except api.UninitializedDatastore: - raise util.Abort(messages.NO_DATA_STORE) + raise util.Abort(messages.REQUIRES_IDS) for i in identifiers: try: rd.remove_item(i) except api.UnknownIdentifier: - raise util.Abort(messages.DELETE_UNKNOWN_ID % i) + raise util.Abort(messages.UNKNOWN_ID % i) except api.AmbiguousIdentifier: - raise util.Abort(messages.DELETE_AMBIGUOUS_ID % i) + raise util.Abort(messages.AMBIGUOUS_ID % i) + +def _edit_command(ui, repo, *args, **opts): + # TODO: require -f to edit some else's item + # TODO: support forcing of plain text + force = opts.pop('force') + identifier = opts.pop('edit') + rd = _get_datastore(ui, repo) + + items = rd.get_items(identifier) + if len(items) == 0: + raise util.Abort(messages.UNKNOWN_ID % identifier) + elif len(items) > 1: + raise util.Abort(messages.AMBIGUOUS_ID % identifier) + item = items[0] + + if item.itemtype == 'comment': + _edit_comment(rd, item, *args, **opts) + elif item.itemtype == 'signoff': + _edit_signoff(rd, item, **opts) _review_effects = { @@ -350,6 +402,8 @@ return _signoff_command(ui, repo, **opts) elif opts.pop('check'): return _check_command(ui, repo, **opts) + elif opts.get('edit'): + return _edit_command(ui, repo, *args, **opts) elif opts.pop('delete'): return _delete_command(ui, repo, *args, **opts) else: @@ -364,6 +418,7 @@ ('r', 'rev', '.', 'the revision to review'), ('d', 'delete', False, 'delete a comment or signoff'), + ('', 'edit', '', 'edit a comment or signoff'), ('', 'check', False, 'check the review status of the given revision'), ('', 'no-nos', False, 'ensure this revision does NOT have signoffs of "no"'), diff -r 03fded64e34e -r dff2e396a30f review/tests/sample_data.py --- a/review/tests/sample_data.py Sat Jun 19 17:39:57 2010 -0400 +++ b/review/tests/sample_data.py Sat Jun 19 17:40:05 2010 -0400 @@ -3,11 +3,13 @@ 'file_two': 'this is another test file', 'long_file': 'a\nb\nc\nd\ne\nf\ng\nh\ni\nj\nk\nl\nm\no\np\nq\nr\ns\nt', 'always_changing': 'this\nfile\nalways\nchanges', + 'always_changing2': 'this\nfile\nalways\nchanges', }, { 'file_one': 'hello again\nworld\nfoo\nbar', 'file_three': 'this is a new file\nfor testing\npurposes\nonly', 'test_dir/test_file': 'this file is inside\nof a directory\n\nponies!', 'long_file': 'a\nb\nc\nd\ne\nf\nX\nh\ni\nj\nk\nl\nY\no\np\nq\nr\ns\nt', - 'always_changing': 'this\nfile\nALWAYS\nchanges', + 'always_changing': 'THIS\nFILE\nALWAYS\nCHANGES', + 'always_changing2': 'THIS\nFILE\nALWAYS\nCHANGES', }, -] \ No newline at end of file +] diff -r 03fded64e34e -r dff2e396a30f review/tests/test_comment.py --- a/review/tests/test_comment.py Sat Jun 19 17:39:57 2010 -0400 +++ b/review/tests/test_comment.py Sat Jun 19 17:40:05 2010 -0400 @@ -3,6 +3,7 @@ from nose import with_setup from util import setup_reviewed_sandbox, teardown_sandbox, review, should_fail_with from util import get_datastore_repo, get_sandbox_repo, get_ui +from util import check_comment_exists_on_line from .. import api, messages @@ -135,12 +136,7 @@ assert a2 in output assert messages.REVIEW_LOG_COMMENT_LINE % 'Test comment one.' in output - # Make sure it's in the correct place - output = output.splitlines() - for n, line in enumerate(output): - if line.startswith('#'): - assert output[n-1].strip().startswith('1') - break + check_comment_exists_on_line(1, files=['file_one'], rev='1') @with_setup(setup_reviewed_sandbox, teardown_sandbox) def test_add_comments_to_file_lines(): @@ -154,12 +150,7 @@ assert a2 in output assert messages.REVIEW_LOG_COMMENT_LINE % 'Test comment one.' in output - # Make sure it's in the correct place - output = output.splitlines() - for n, line in enumerate(output): - if line.startswith('#'): - assert output[n-1].strip().startswith('2') - break + check_comment_exists_on_line(2, files=['file_one'], rev='1') @with_setup(setup_reviewed_sandbox, teardown_sandbox) def test_add_comments_to_file_in_subdir(): diff -r 03fded64e34e -r dff2e396a30f review/tests/test_delete.py --- a/review/tests/test_delete.py Sat Jun 19 17:39:57 2010 -0400 +++ b/review/tests/test_delete.py Sat Jun 19 17:40:05 2010 -0400 @@ -31,7 +31,7 @@ review(rev=rev, comment=True, message='test') i = get_identifiers(rev)[0] - output = review(rev=rev, delete=True, args=[i]) + output = review(delete=True, args=[i]) assert not output output = review(rev=rev, verbose=True) assert '(%s)\n' % i not in output @@ -40,7 +40,7 @@ review(rev=rev, comment=True, message='test3') i1, i2 = get_identifiers(rev) - output = review(rev=rev, delete=True, args=[i1]) + output = review(delete=True, args=[i1]) assert not output output = review(rev=rev, verbose=True) assert '(%s)\n' % i1 not in output @@ -49,7 +49,7 @@ review(rev=rev, comment=True, message='test4') i1, i2 = get_identifiers(rev) - output = review(rev=rev, delete=True, args=[i1, i2]) + output = review(delete=True, args=[i1, i2]) assert not output output = review(rev=rev, verbose=True) assert '(%s)\n' % i1 not in output diff -r 03fded64e34e -r dff2e396a30f review/tests/test_edit.py --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/review/tests/test_edit.py Sat Jun 19 17:40:05 2010 -0400 @@ -0,0 +1,133 @@ +import time + +from nose import with_setup +from util import setup_reviewed_sandbox, teardown_sandbox, review, should_fail_with +from util import get_identifiers, check_comment_exists_on_line + +from .. import messages + +@with_setup(setup_reviewed_sandbox, teardown_sandbox) +def test_edit_invalid(): + should_fail_with(messages.UNKNOWN_ID % 'z', edit='z') + + review(comment=True, message='test') + + should_fail_with(messages.UNKNOWN_ID % 'z', edit='z') + + # Use the pidgeonhole princicple to create ambiguous identifiers. + for i in range(17): + review(comment=True, message='test%d' % i) + + ids = get_identifiers() + id_map = {} + for i in ids: + id_map[i[0]] = id_map.get(i[0], 0) + 1 + i = str(filter(lambda k: id_map[k] > 1, id_map.keys())[0]) + + should_fail_with(messages.AMBIGUOUS_ID % i, edit=i) + +@with_setup(setup_reviewed_sandbox, teardown_sandbox) +def test_touch_comment(): + def t(rev): + review(rev=rev, comment=True, message='test', args=['always_changing'], lines='1') + i = get_identifiers(rev)[0] + + # This sucks, but we need to do it to support testing "touch" edits. + time.sleep(1.1) + + output = review(edit=i) + assert not output + output = review(rev=rev, verbose=True) + assert '(%s)' % i not in output + assert len(get_identifiers(rev)) == 1 + assert messages.REVIEW_LOG_COMMENT_LINE % 'test' in output + + check_comment_exists_on_line(1, files=['always_changing'], rev='1') + t('1') + t('0') + +@with_setup(setup_reviewed_sandbox, teardown_sandbox) +def test_edit_comment_message(): + def t(rev): + review(rev=rev, comment=True, message='test') + i = get_identifiers(rev)[0] + + output = review(edit=i, message='edited') + assert not output + output = review(rev=rev, verbose=True) + assert '(%s)' % i not in output + assert len(get_identifiers(rev)) == 1 + assert messages.REVIEW_LOG_COMMENT_LINE % 'test' not in output + assert messages.REVIEW_LOG_COMMENT_LINE % 'edited' in output + t('.') + t('0') + +@with_setup(setup_reviewed_sandbox, teardown_sandbox) +def test_edit_comment_lines(): + def t(rev): + review(rev=rev, comment=True, message='test', args=['always_changing'], lines='1') + i = get_identifiers(rev)[0] + + output = review(edit=i, lines='3') + assert not output + output = review(rev=rev, verbose=True) + assert '(%s)' % i not in output + assert len(get_identifiers(rev)) == 1 + assert messages.REVIEW_LOG_COMMENT_LINE % 'test' in output + check_comment_exists_on_line(3, files=['always_changing'], rev=rev) + + i = get_identifiers(rev)[0] + + output = review(edit=i, lines='1,2') + assert not output + output = review(rev=rev, verbose=True) + assert '(%s)' % i not in output + assert len(get_identifiers(rev)) == 1 + assert messages.REVIEW_LOG_COMMENT_LINE % 'test' in output + check_comment_exists_on_line(2, files=['always_changing'], rev=rev) + t('1') + t('0') + +@with_setup(setup_reviewed_sandbox, teardown_sandbox) +def test_edit_comment_filename(): + def t(rev): + review(rev=rev, comment=True, message='test', args=['always_changing'], lines='1') + i = get_identifiers(rev)[0] + + output = review(edit=i, args=['always_changing2']) + assert not output + + output = review(rev=rev, verbose=True, args=['always_changing']) + assert '(%s)' % i not in output + assert len(get_identifiers(rev, files=['always_changing'])) == 0 + assert messages.REVIEW_LOG_COMMENT_LINE % 'test' not in output + + output = review(rev=rev, verbose=True, args=['always_changing2']) + assert '(%s)' % i not in output + assert len(get_identifiers(rev, files=['always_changing2'])) == 1 + assert messages.REVIEW_LOG_COMMENT_LINE % 'test' in output + t('1') + t('0') + +@with_setup(setup_reviewed_sandbox, teardown_sandbox) +def test_edit_comment_everything(): + def t(rev): + review(rev=rev, comment=True, message='test', args=['always_changing'], lines='1') + i = get_identifiers(rev)[0] + + output = review(edit=i, args=['always_changing2'], message='edited', lines='2') + assert not output + + output = review(rev=rev, verbose=True, args=['always_changing']) + assert '(%s)' % i not in output + assert len(get_identifiers(rev, files=['always_changing'])) == 0 + assert messages.REVIEW_LOG_COMMENT_LINE % 'test' not in output + + output = review(rev=rev, verbose=True, args=['always_changing2']) + assert '(%s)' % i not in output + assert len(get_identifiers(rev, files=['always_changing2'])) == 1 + assert messages.REVIEW_LOG_COMMENT_LINE % 'test' not in output + assert messages.REVIEW_LOG_COMMENT_LINE % 'edited' in output + check_comment_exists_on_line(2, files=['always_changing'], rev=rev) + t('1') + t('0') diff -r 03fded64e34e -r dff2e396a30f review/tests/util.py --- a/review/tests/util.py Sat Jun 19 17:39:57 2010 -0400 +++ b/review/tests/util.py Sat Jun 19 17:40:05 2010 -0400 @@ -134,7 +134,24 @@ a1, a2 = (messages.REVIEW_LOG_COMMENT_AUTHOR % '|').split('|') s1, s2 = (messages.REVIEW_LOG_SIGNOFF_AUTHOR % ('|', 'neutral')).split('|') -def get_identifiers(rev='.'): +def get_identifiers(rev='.', files=[]): return [l.split(' ')[-1].strip('()\n') - for l in review(rev=rev, verbose=True).splitlines() + for l in review(rev=rev, verbose=True, args=files).splitlines() if (a1 in l and a2 in l) or (s1 in l and s2 in l)] + + +COMMENT_LINE_ERROR = '''\ +Expected a comment on line %d: + +%s +%s +''' +def check_comment_exists_on_line(n, files=[], rev='.'): + output = review(rev=rev, args=files).splitlines() + for i, line in enumerate(output): + if line.startswith('#'): + assert output[i-1].strip().startswith(str(n)), \ + COMMENT_LINE_ERROR % (n, output[i-1].rstrip(), + output[i].rstrip()) + break +