martinvonz / jj

A Git-compatible VCS that is both simple and powerful
https://martinvonz.github.io/jj/
Apache License 2.0
8.27k stars 281 forks source link

FR: Rename `conflict()` revset function to `conflicts()` #4122

Open cenviity opened 2 months ago

cenviity commented 2 months ago

Is your feature request related to a problem? Please describe.

The conflict() revset function may return multiple commits with conflicts, so the plural form conflicts() should be used to better indicate this. There is currently no conflicts() function, so an error occurs if the user tries to use it in a revset.

Describe the solution you'd like

Describe alternatives you've considered

We could remain with the status quo, but this seems inconsistent with the naming of other revset functions:

yuja commented 2 months ago

Our revset function naming is somewhat inconsistent, but I think conflict() belongs to the group that the function name represents a property of matching revisions:

OTOH, parents(), children(), etc. are short for "parent" revisions, "child" revisions, ...

cenviity commented 2 months ago

I see your reasoning, although since a commit may contain multiple conflicts I would still read it as 'has conflicts()' rather. Or perhaps 'is conflicted()'?

yuja commented 2 months ago

I'm not sure if adjective use of "conflict" is okay. I'm not against renaming to conflicted/conflicts/conflicting? if that sounds strictly better. We'll also need to update templater and some other internal APIs.

cenviity commented 2 months ago

In fact, we could consider the functions with no parameters as taking an implicit repo and returning revsets that are almost like attributes of the repo. For example:

In that view, conflicts() returns conflict commits of the repo.

I'm not sure if adjective use of "conflict" is okay. I'm not against renaming to conflicted/conflicts/conflicting? if that sounds strictly better. We'll also need to update templater and some other internal APIs.

I prefer conflicts() over conflicted(), since the plural form reflects the idea of possibly returning multiple commits. I will draft a PR when I have time.

yuja commented 2 months ago

fwiw, I feel tags(), branches(), merges() are a bit odd because they don't return a list of tags, etc., but that might be because I'm coming from Mercurial. https://repo.mercurial-scm.org/hg/help/revsets

Anyway, I'm not a right person to decide naming thingy.

essiene commented 1 month ago

Is this settled? I'm picking up low-hanging fruit and this is as low hanging as it gets. Though I'm not sure if we've settled on the rename?

yuja commented 1 month ago

Is this settled?

No, I don't think it is. FWIW, someone typoed merges() as merge() on Discord, and I think merge() makes more sense. In my mental model, it is a predicate to test if "this revision is a merge."

martinvonz commented 1 month ago

Maybe we should just define singular and plural versions as aliases for any function where users are likely to use the wrong form?

yuja commented 1 month ago

Maybe we should just define singular and plural versions as aliases for any function

Wouldn't that create too many aliases? I prefer the current behavior that merges() is suggested if I used merge().

martinvonz commented 1 month ago

Probably true. I'm fine with renaming all to plural or renaming merges() to merge(), btw. I don't have a strong opinion. I feel like I would instinctively use plural conflicts(), fwiw.

essiene commented 1 month ago

I'll send a PR to rename singular to plural where it makes sense.