Page MenuHomePhabricator

perf: use the `perf--` prefix for perf command
ClosedPublic

Authored by marmoute on Dec 4 2020, 4:25 AM.

Details

Summary

This is the one command namespace where they should not be any ambiguity about
command that should be in it. The perf extensions is only adding performance
related command.

so this is a good ground to start putting dash folding to the tests.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Dec 4 2020, 4:25 AM
pulkit accepted this revision.Dec 4 2020, 2:19 PM
This revision is now accepted and ready to land.Dec 4 2020, 2:19 PM
baymax updated this revision to Diff 24070.Dec 6 2020, 9:37 AM
Alphare accepted this revision.Dec 14 2020, 5:01 AM
baymax retitled this revision from perf: use the `perf-` prefix for perf command to perf: use the `perf--` prefix for perf command.Dec 27 2020, 1:42 PM
baymax updated this revision to Diff 24513.

โœ… refresh by Heptapod after a successful CI run (๐Ÿ™ ๐Ÿ’š)

durin42 added inline comments.
contrib/perf.py
747

Why is this two dashes and not one?

(Here and elsewhere. I was surprised to see two and not one, and I don't know what that signifies...)

marmoute requested review of this revision.Jan 13 2021, 11:15 PM
marmoute added inline comments.
contrib/perf.py
747

I took a time machine and answer that question last month ;-)

https://www.mercurial-scm.org/pipermail/mercurial-devel/2020-December/144168.html

mharbison72 added inline comments.
contrib/perf.py
747

I don't want to endlessly bikeshed this, but the double dash really bothers me for some reason (maybe because it looks like a typo). If we really need a marker for a namespace, maybe ":" would work? But I don't like that too much either, as I can't think of anything that uses that style.

Big picture, why do we need to be able to tell namespace parts from command parts? My assumption was that namespaces would simply group commands in a related area (if a command list was sorted), or for a theoretical hg help $namespace. But presumably if we're defining namespaces, the code for that version of hg will know what namespaces are defined, and it's a simple prefix check without spilling implementation details into the UI.

I'm fine with this as-is, but I am sympathetic to the concern that the -- looks like a mistake. If we want to change it we can, since we've got a full release cycle ahead of us before this is binding.

marmoute added inline comments.Jan 20 2021, 2:29 PM
contrib/perf.py
747

I don't want to endlessly bikeshed this,

Now is precisely the good time to bikeshed this, (and as Augie pointed, we now have 3.5 month to change whatever pick we get).

but the double dash really bothers me for some reason (maybe because it looks like a typo).

The more I have been looking at it, the more I agree. It look as if a space was forgotten before a --option. However we need something better.

maybe ":" would work? But I don't like that too much either, as I can't think of anything that uses that style.

Hum why not, we could give it a try (admin:strip). Maybe . (admin.strip).

Big picture, why do we need to be able to tell namespace parts from command parts?

To make sure the user understand they are stepping in an entire different realm with different rules.

In the standard namespace (no prefix) we garantee (or aims at) the lack of footgun and excellent backward compatibility.

The debug namespace should not be somewhere any normal user ever have to go (at least without a responsible staff member)

The admin namespace will have its own rules and its own "be careful, this is not a "normal" situation to be there.

So we want something more distincting than just a simple - that we could have for "command group" or readability like phab-send, phab-read, etc.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

Big picture, why do we need to be able to tell namespace parts from command parts?

To make sure the user understand they are stepping in an entire different realm with different rules.
In the standard namespace (no prefix) we garantee (or aims at) the lack of footgun and excellent backward compatibility.
The debug namespace should not be somewhere any normal user ever have to go (at least without a responsible staff member)
The admin namespace will have its own rules and its own "be careful, this is not a "normal" situation to be there.
So we want something more distincting than just a simple - that we could have for "command group" or readability like phab-send, phab-read, etc.

So it sounds like more of a convention for these behavior properties than say, commands in the admin group requiring --yes-i-know-this-may-eat-my-data. We'll have to have to figure out how to convey that somehow. I see admin, for example, and think "I'm in charge of making this work for my group, and it sounds like it does what I want, so it's for me and just some extra characters to type". I don't have any suggestions on how to highlight these behaviors ATM.

but the double dash really bothers me for some reason (maybe because it looks like a typo).

The more I have been looking at it, the more I agree. It look as if a space was forgotten before a --option. However we need something better.

maybe ":" would work? But I don't like that too much either, as I can't think of anything that uses that style.

Hum why not, we could give it a try (admin:strip). Maybe . (admin.strip).

If simple convention is driving this, I'm not sure that a one character change from - to : or . stands out in a meaningful way. (IOW, why is changing it better?) Even though I've never seen . in arg names, I do like that it fits well with existing multi-level config options. If a double character is important, maybe ::? (Although maybe that's too much like C++ namespaces, and maybe why my mind went to : as a namespace indicator.)

Big picture, why do we need to be able to tell namespace parts from command parts?

To make sure the user understand they are stepping in an entire different realm with different rules.
In the standard namespace (no prefix) we garantee (or aims at) the lack of footgun and excellent backward compatibility.
The debug namespace should not be somewhere any normal user ever have to go (at least without a responsible staff member)
The admin namespace will have its own rules and its own "be careful, this is not a "normal" situation to be there.
So we want something more distincting than just a simple - that we could have for "command group" or readability like phab-send, phab-read, etc.

So it sounds like more of a convention for these behavior properties than say, commands in the admin group requiring --yes-i-know-this-may-eat-my-data. We'll have to have to figure out how to convey that somehow. I see admin, for example, and think "I'm in charge of making this work for my group, and it sounds like it does what I want, so it's for me and just some extra characters to type". I don't have any suggestions on how to highlight these behaviors ATM.

I don't think the command in the admin namespace should especially be footgun, but they will be "less usual". If we only get the "I'm in charge of making this work for my group" people to use them, we reached our intended goal.

but the double dash really bothers me for some reason (maybe because it looks like a typo).

The more I have been looking at it, the more I agree. It look as if a space was forgotten before a --option. However we need something better.

maybe ":" would work? But I don't like that too much either, as I can't think of anything that uses that style.

Hum why not, we could give it a try (admin:strip). Maybe . (admin.strip).

If simple convention is driving this, I'm not sure that a one character change from - to : or . stands out in a meaningful way. (IOW, why is changing it better?) Even though I've never seen . in arg names, I do like that it fits well with existing multi-level config options. If a double character is important, maybe ::? (Although maybe that's too much like C++ namespaces, and maybe why my mind went to : as a namespace indicator.)

Actually, I think I like :: is a common way to refer to namespace so it feel appropriate.

The -- also bothers me and I came here to see if it had been discussed before accepting this patch.

I like : or :: as the namespace separator. I suppose we have a few months to change things before this ships. So I'll probably stamp this.