On Wed, Mar 22, 2017 at 7:38 AM, Ryan McElroy <r...@fb.com> wrote: > On 3/22/17 6:49 AM, Gregory Szorc wrote: > >> # HG changeset patch >> # User Gregory Szorc <gregory.sz...@gmail.com> >> # Date 1490165337 25200 >> # Tue Mar 21 23:48:57 2017 -0700 >> # Node ID 80ca2bee4a06887f918e3328b3f005e4c1cb1ab1 >> # Parent ae796e23fd42b036352b298f570af8949c2db2d9 >> show: new extension for displaying various repository data >> >> Currently, Mercurial has a number of commands to show information. And, >> there are features coming down the pipe that will introduce more >> commands for showing information. >> >> Currently, when introducing a new class of data or a view that we >> wish to expose to the user, the strategy is to introduce a new command >> or overload an existing command, sometimes both. For example, there is >> a desire to formalize the wip/smartlog/underway/mine functionality that >> many have devised. There is also a desire to introduce a "topics" >> concept. Others would like views of "the current stack." In the >> current model, we'd need a new command for wip/smartlog/etc (that >> behaves a lot like a pre-defined alias of `hg log`). For topics, >> we'd likely overload `hg topic[s]` to both display and manipulate >> topics. >> >> Adding new commands for every pre-defined query doesn't scale well >> and pollutes `hg help`. Overloading commands to perform read-only and >> write operations is arguably an UX anti-pattern: while having all >> functionality for a given concept in one command is nice, having a >> single command doing multiple discrete operations is not. Furthermore, >> a user may be surprised that a command they thought was read-only >> actually changes something. >> >> We discussed this at the Mercurial 4.0 Sprint in Paris and decided that >> having a single command where we could hang pre-defined views of >> various data would be a good idea. Having such a command would: >> >> * Help prevent an explosion of new query-related commands >> * Create a clear separation between read and write operations >> (mitigates footguns) >> * Avoids overloading the meaning of commands that manipulate data >> (bookmark, tag, branch, etc) (while we can't take away the >> existing behavior for BC reasons, we now won't introduce this >> behavior on new commands) >> > I'm not convinced that these "footguns" are a bad ui though. I certainly > would be less comfortable asking people who are used to writing "hg boo" to > change to "hg show bookmarks" (see comment on > > * Allows users to discover informational views more easily by >> aggregating them in a single location >> * Lowers the barrier to creating the new views (since the barrier >> to creating a top-level command is relatively high) >> >> So, this commit introduces the `hg show` command via the "show" >> extension. This command accepts a positional argument of the >> "view" to show. New views can be registered with a decorator. To >> prove it works, we implement the "bookmarks" view, which shows a >> table of bookmarks and their associated nodes. >> >> We introduce a new style to hold everything used by `hg show`. >> > Ah, this answers some of my inline questions below about the new styles. > > >> For our initial bookmarks view, the output varies from `hg bookmarks`: >> >> * Padding is performed in the template itself as opposed to Python >> * Revision integers are not shown >> * shortest() is used to display a 5 character node by default (as >> opposed to static 12 characters) >> >> I chose to implement the "bookmarks" view first because it is simple >> and shouldn't invite too much bikeshedding that detracts from the >> evaluation of `hg show` itself. But there is an important point >> to consider: we now have 2 ways to show a list of bookmarks. I'm not >> a fan of introducing multiple ways to do very similar things. So it >> might be worth discussing how we wish to tackle this issue for >> bookmarks, tags, branches, MQ series, etc. >> > I'm pretty concerned about this too. I don't have an answer right now but > I'll think about it. > > >> I also made the choice of explicitly declaring the default show >> template not part of the standard BC guarantees. History has shown >> that we make mistakes and poor choices with output formatting but >> can't fix these mistakes later because random tools are parsing >> output and we don't want to break these tools. Optimizing for human >> consumption is one of my goals for `hg show`. So, by not covering >> the formatting as part of BC, the barrier to future change is much >> lower and humans benefit. >> > This is awesome. See inline comments for additional idea. First > "porcelain" hg command :-p. > > >> There are some improvements that can be made to formatting. For >> example, we don't yet use label() in the templates. We obviously >> want this for color. But I'm not sure if we should reuse the existing >> log.* labels or invent new ones. I figure we can punt that to a >> follow-up. >> > Seems that this patch is good for collecting shed colors, but this would > probably want to be broken up into a series before it actually goes in, > right?
I figured this patch would land as a big chunk and we'd iterate using the normal process. I think trying to split the initial landing into multiple parts is a bit difficult because there's a lot to take in. But if people want me to split it, I can. > > > >> At the aforementioned Sprint, we discussed and discarded various >> alternatives to `hg show`. >> >> We considered making `hg log <view>` perform this behavior. The main >> reason we can't do this is because a positional argument to `hg log` >> can be a file path and if there is a conflict between a path name and >> a view name, behavior is ambiguous. We could have introduced >> `hg log --view` or similar, but we felt that required too much typing >> (we don't want to require a command flag to show a view) and wasn't >> very discoverable. Furthermore, `hg log` is optimized for showing >> changelog data and there are things that `hg display` could display >> that aren't changelog centric. >> >> There were concerns about using "show" as the command name. >> >> Some users already have a "show" alias that is similar to `hg export`. >> >> There were also concerns that Git users adapted to `git show` would >> be confused by `hg show`'s different behavior. The main difference >> here is `git show` prints an `hg export` like view of the current >> commit by default and `hg show` requires an argument. `git show` >> can also display any Git object. `git show` does not support >> displaying more complex views: just single objects. If we >> implemented `hg show <hash>` or `hg show <identifier>`, `hg show` >> would be a superset of `git show`. Although, I'm hesitant to do that >> at this time because I view `hg show` as a higher-level querying >> command and there are namespace collisions between valid identifiers >> and registered views. >> >> There is also a prefix collision with `hg showconfig`, which is an >> alias of `hg config`. >> > I'm not too concerned about this. As I said in the other thread, fb has a > show extension as well. It's basically a thin wrapper around `hg log -vpr` > that allows a few additional flags and has a default '.' parameter: > https://bitbucket.org/facebook/hg-experimental/src/default/ > hgext3rd/show.py > > >> We also considered `hg view`, but that is already used by the "hgk" >> extension. >> > Meh. Can we deprecate hgk? (Serious question: I don't know how widely it's > used. We seem to have it installed at FB but it seems totally broken and > nobody has ever complained so it seems that out of thousands of users > nobody has ever noticed it's there). > I'm not opposed to renaming this `hg view`. But I'd prefer that be done as a follow-up because I want to get this "framework" landed! > > >> `hg display` was also proposed at one point. It has a prefix collision >> with `hg diff`. General consensus was "show" or "view" are the best >> verbs. And since "view" was taken, "show" was chosen. >> > Seems reasonable. Feel free to ignore my bikeshedding above. I'm okay with > show overall. > > >> diff --git a/contrib/wix/templates.wxs b/contrib/wix/templates.wxs >> --- a/contrib/wix/templates.wxs >> +++ b/contrib/wix/templates.wxs >> @@ -32,6 +32,7 @@ >> <File Name="map-cmdline.changelog" KeyPath="yes" /> >> <File Name="map-cmdline.compact" /> >> <File Name="map-cmdline.default" /> >> + <File Name="map-cmdline.show" /> >> <File Name="map-cmdline.bisect" /> >> <File Name="map-cmdline.xml" /> >> <File Name="map-cmdline.status" /> >> > What are these files? I'm not familiar with what's going on here. The map-cmdline.* files are various styles that can be used with -T. I invented a new style for this command. I'm not sure that if this is proper. Yuya already directed me to not have various template names conflict with built-in templates (hence the "show*" prefixing). So perhaps I could roll these templates into map-cmdline.default? > > > diff --git a/hgext/show.py b/hgext/show.py >> new file mode 100644 >> --- /dev/null >> +++ b/hgext/show.py >> @@ -0,0 +1,115 @@ >> +# show.py - Extension implementing `hg show` >> +# >> +# Copyright 2017 Gregory Szorc <gregory.sz...@gmail.com> >> +# >> +# This software may be used and distributed according to the terms of the >> +# GNU General Public License version 2 or any later version. >> + >> +"""unified command to show various repository information (EXPERIMENTAL) >> + >> +This extension provides the :hg:`show` command, which provides a central >> +command for displaying commonly-accessed repository data and views of >> that >> +data. >> +""" >> + >> +from __future__ import absolute_import >> + >> +from mercurial.i18n import _ >> +from mercurial import ( >> + cmdutil, >> + commands, >> + error, >> + registrar, >> +) >> + >> +# Note for extension authors: ONLY specify testedwith = >> 'ships-with-hg-core' for >> +# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should >> +# be specifying the version(s) of Mercurial they are tested with, or >> +# leave the attribute unspecified. >> +testedwith = 'ships-with-hg-core' >> + >> +cmdtable = {} >> +command = cmdutil.command(cmdtable) >> + >> +class showcmdfunc(registrar._funcregistrarbase): >> + """Register a function to be invoked for an `hg show <thing>`.""" >> + _docformat = '%s -- %s' >> > Unused variable? > > + >> + def _extrasetup(self, name, func, fmtopic=None): >> + func._fmtopic = fmtopic >> > Docstring please -- I have no idea what this is for. Also, it's unused? > This stuff is used. I'll add comments. > > + >> +showview = showcmdfunc() >> + >> +@command('show', commands.formatteropts, _('[VIEW]')) >> +def show(ui, repo, view=None, template=None): >> + """show various repository information >> + >> + A requested view of repository data is displayed. >> + >> + If no view is requested, the list of available views is shown. >> > (and we abort) > > + >> + .. note:: >> + >> + The default output from this command is not covered under >> Mercurial's >> + default backwards-compatible mechanism (which puts an emphasis on >> + not changing behavior). This means output from this command may >> change >> + in any future release. However, the values fed to the formatter >> are >> + covered under the default backwards-compatible mechanism. >> + >> + Automated consumers of this command should specify an explicit >> template >> + via ``-T/--template`` to guarantee output is stable. >> > YES! I love it. > > We should consider aborting if HGPLAIN=1 and no template is specified, in > fact! > > + >> + List of available views: >> + >> + """ >> + views = showview._table >> + >> + if not view: >> + ui.write(_('available views:\n')) >> + ui.write('\n') >> > Should this use a formatter? I could imagine wanting to consume available > views in json, for example. > Probably. But I'm not sure how to do it though (I'm not sure how to get the header and footer printed while also printing multiple items). I've added a TODO in the next version. > > + >> + for name, func in sorted(views.items()): >> + ui.write(_('%s\n') % func.__doc__) >> > The entire docstring seems like too much. We should probably have only the > first line of the docstring here. Also, you don't use the name. Shouldn't > the name be here as well? > But it's not the full docstring: that _docformat variable does magic ;) > > Nitpick: I don't think it's useful to internationalize '%s\n'. > > + >> + ui.write('\n') >> + raise error.Abort(_('no view requested'), >> + hint=_('use `hg show <view>` to choose a >> view')) >> > This hint format is different that what I usually see in hg. Shouldn't it > be 'hg show VIEW' so it's consistent with our other command help output? > Also, we tend to not use backticks, right? > > + >> + if view not in views: >> + raise error.Abort(_('unknown view: %s') % view, >> + hint=_('run `hg show` to see available views')) >> > We should probably do prefix matching to be consistent with other commands > in hg (and to help out lazy typers). > Agreed. I'll add a TODO (I don't want to bloat scope too much). > > + >> + template = template or 'show' >> + fmtopic = 'show%s' % views[view]._fmtopic >> + >> + ui.pager('show') >> > Why is this down here but not up where we print a list of views? > Will fix. > > + with ui.formatter(fmtopic, {'template': template}) as fm: >> + return views[view](ui, repo, fm) >> + >> +@showview('bookmarks', fmtopic='bookmarks') >> +def showbookmarks(ui, repo, fm): >> + """bookmarks and their associated changeset""" >> + marks = repo._bookmarks >> + if not len(marks): >> + ui.write(_('(no bookmarks set)\n')) >> + return >> + >> + active = repo._activebookmark >> + longest = max(len(b) for b in marks) >> + >> + for bm, node in sorted(marks.items()): >> + fm.startitem() >> + fm.context(ctx=repo[node]) >> + fm.write('bookmark', '%s', bm) >> + fm.write('node', fm.hexfunc(node), fm.hexfunc(node)) >> + fm.data(active=bm == active, >> + _longestlen=longest) >> > _longest should probably be _longestname, because shortened nodes can also > have interesting length properties -- and we may want to pass _longestnode > in the future if there is more data we want to show here and we want to pad > nicely. > Agreed. Although the longest shorted node is a bit difficult to implement because AFAICT there is no obvious function to call. I'll TODO that one. > > + >> +# Adjust the docstring of the show command so it shows all registered >> views. >> +# This is a bit hacky because it runs at the end of module load. When >> moved >> +# into core or when another extension wants to provide a view, we'll need >> +# to do this more robustly. >> +longest = max(map(len, showview._table.keys())) >> +for key in sorted(showview._table.keys()): >> + cmdtable['show'][0].__doc__ += ' %s %s\n' % ( >> + key.ljust(longest), showview._table[key]._origdoc) >> > Yuck! Surely we can do better with a formatter and padding? At the very > least, we can just compute this when we need to print it out and not at > module load time. But it would be better to use the formatter if possible, > then we get things like JSON for free. > Tell me about it. This ideally wants some kind of hook in the help mechanism. I didn't want to bloat scope and implement that at this juncture. I've added a TODO. > > diff --git a/mercurial/templates/map-cmdline.show >> b/mercurial/templates/map-cmdline.show >> new file mode 100644 >> --- /dev/null >> +++ b/mercurial/templates/map-cmdline.show >> @@ -0,0 +1,2 @@ >> +# TODO add label() once we figure out which namespace the labels belong >> on. >> +showbookmarks = '{if(active, "*", " ")} {pad(bookmark, _longestlen + >> 4)}{shortest(node, 5)}\n' >> > What is this file? > Our custom style (which may or may not be needed). > > diff --git a/tests/test-command-template.t b/tests/test-command-template.t >> --- a/tests/test-command-template.t >> +++ b/tests/test-command-template.t >> @@ -1108,11 +1108,11 @@ Error if no style: >> $ hg log --style notexist >> abort: style 'notexist' not found >> - (available styles: bisect, changelog, compact, default, phases, >> status, xml) >> + (available styles: bisect, changelog, compact, default, phases, show, >> status, xml) >> [255] >> $ hg log -T list >> - available styles: bisect, changelog, compact, default, phases, status, >> xml >> + available styles: bisect, changelog, compact, default, phases, show, >> status, xml >> abort: specify a template >> [255] >> > I'm confused by these test changes. Why does show show up here? Because this is listing available styles (the map-cmdline.* files). > > > diff --git a/tests/test-log.t b/tests/test-log.t >> --- a/tests/test-log.t >> +++ b/tests/test-log.t >> @@ -148,7 +148,7 @@ log on directory >> $ hg log -f -l1 --style something >> abort: style 'something' not found >> - (available styles: bisect, changelog, compact, default, phases, >> status, xml) >> + (available styles: bisect, changelog, compact, default, phases, show, >> status, xml) >> [255] >> -f, phases style >> diff --git a/tests/test-show.t b/tests/test-show.t >> new file mode 100644 >> --- /dev/null >> +++ b/tests/test-show.t >> @@ -0,0 +1,111 @@ >> + $ cat >> $HGRCPATH << EOF >> + > [extensions] >> + > show = >> + > EOF >> + >> +No arguments shows available views >> + >> + $ hg init empty >> + $ cd empty >> + $ hg show >> + available views: >> + >> + bookmarks -- bookmarks and their associated changeset >> + >> + abort: no view requested >> + (use `hg show <view>` to choose a view) >> + [255] >> + >> +`hg help show` prints available views >> + >> + $ hg help show >> + hg show [VIEW] >> > If we abort otherwise, I wouldn't say that VIEW is optional. I guess I'm > against the abort overall, though... > > > + >> + show various repository information >> + >> + A requested view of repository data is displayed. >> + >> + If no view is requested, the list of available views is shown. >> + >> + Note: >> + The default output from this command is not covered under >> Mercurial's >> + default backwards-compatible mechanism (which puts an emphasis >> on not >> + changing behavior). This means output from this command may >> change in >> + any future release. However, the values fed to the formatter are >> + covered under the default backwards-compatible mechanism. >> + >> + Automated consumers of this command should specify an explicit >> template >> + via "-T/--template" to guarantee output is stable. >> + >> + List of available views: >> + >> + bookmarks bookmarks and their associated changeset >> + >> + (use 'hg help -e show' to show help for the show extension) >> + >> + options: >> + >> + (some details hidden, use --verbose to show complete help) >> + >> +Unknown view prints error >> + >> + $ hg show badview >> + abort: unknown view: badview >> + (run `hg show` to see available views) >> + [255] >> + >> + $ cd .. >> + >> +bookmarks view with no bookmarks prints empty message >> + >> + $ hg init books >> + $ cd books >> + $ touch f0 >> + $ hg -q commit -A -m initial >> + >> + $ hg show bookmarks >> + (no bookmarks set) >> + >> +bookmarks view shows bookmarks in an aligned table >> + >> + $ echo book1 > f0 >> + $ hg commit -m 'commit for book1' >> + $ echo book2 > f0 >> + $ hg commit -m 'commit for book2' >> + >> + $ hg bookmark -r 1 book1 >> + $ hg bookmark a-longer-bookmark >> + >> + $ hg show bookmarks >> + * a-longer-bookmark 7b570 >> + book1 b757f >> + >> +Bookmarks plain view >> + >> + $ HGPLAIN= hg show bookmarks >> + * a-longer-bookmark 7b570 >> + book1 b757f >> > Let's make this abort instead. > > > + >> +A custom bookmarks template works >> + >> + $ hg show bookmarks -T '{node} {bookmark} {active}\n' >> + 7b5709ab64cbc34da9b4367b64afff47f2c4ee83 a-longer-bookmark True >> + b757f780b8ffd71267c6ccb32e0882d9d32a8cc0 book1 False >> + >> +bookmarks JSON works >> + >> + $ hg show bookmarks -T json >> + [ >> + { >> + "active": true, >> + "bookmark": "a-longer-bookmark", >> + "node": "7b5709ab64cbc34da9b4367b64afff47f2c4ee83" >> + }, >> + { >> + "active": false, >> + "bookmark": "book1", >> + "node": "b757f780b8ffd71267c6ccb32e0882d9d32a8cc0" >> + } >> + ] >> + >> + $ cd .. >> > >
_______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel