Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: complete: complete -T template aliases #5541

Merged
merged 1 commit into from
Feb 1, 2025

Conversation

jakobhellermann
Copy link
Contributor

@jakobhellermann jakobhellermann commented Jan 31, 2025

based on #5539

image

It would be nice to filter them down to only applicable ones, but I don't think that type information is present in the toml definitions.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@jakobhellermann
Copy link
Contributor Author

We could also emit a description for the template, but it would only have room for one line, and I don't think that provides much value.

image

I suppose another option would be only to display the help text for one-line templates, which is better, but probably not worth the visual clutter over the simple horizontal list of all templates.

image

@ilyagr
Copy link
Contributor

ilyagr commented Feb 1, 2025

I'm not sure how major this is, but these suggestions include things like jj log -T builtin_op_log_compact that result in an error.

A hacky and impefect solution might be to filter the log templates by the regex ^builtin_log.

I wonder if we could/should reorganize templates by the kind of object they take, so that this would be op_log.builtin_compact. This might be a bit of a project, it would also raise the questions of what to do about template aliases, and whether that should be referred to as jj op log -T op_log.builtin_compact or whether jj op log should assume the op_log. prefix.

@jakobhellermann
Copy link
Contributor Author

A hacky and imperfect solution might be to filter the log templates by the regex ^builtin_log.

Could do that, although you would lose user defined template aliases with another prefix (how common is that?) and you'd still get invalid builtin_log_node*.

I wonder if we could/should reorganize templates by the kind of object they take, so that this would be op_log.builtin_compact

Hm. My gut reaction says that I would prefer to have flat template-aliases, that makes it easier to think about them. It's just a set of defined variables/functions.
It's non-obvious that you should use [template-aliases.op_log] my_custom_log = '...' so users might just not do that and miss out on completion.

Another option would be to introduce some understanding of types to the functions and symbol, but that would be an even larger undertaking and it's not clear that it is worth the complexity

Sketch of adding type information to templates
'commit_timestamp(commit)' = 'commit.committer().timestamp()'

could be written as

'commit_timestamp(commit: Commit)' = 'commit.committer().timestamp()'

and

builtin_log_compact = '''
if(root,
  format_root_commit(self),
  ...
)
'''

might become

'builtin_log_compact(self: Commit)' = '''
if(root,
  format_root_commit(self),
  foo # implicitly self.foo
  ...
)
'''

And then, only aliases that have a type specified and match the expected type are completed.
This would potentially useful for other features (autocomplete in the config file via LSP injection?, better error messages due to earlier type checking, ??).

Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to add some metadata (e.g. self type, short documentation, etc.) to the builtin templates, but I don't have any concrete idea right now.

cli/src/commands/config/list.rs Outdated Show resolved Hide resolved
cli/tests/[email protected] Outdated Show resolved Hide resolved
@jakobhellermann
Copy link
Contributor Author

I've opened a followup issues for potential type metadata in the aliases: #5551

@jakobhellermann jakobhellermann added this pull request to the merge queue Feb 1, 2025
Merged via the queue into jj-vcs:main with commit f7429f2 Feb 1, 2025
19 checks passed
@jakobhellermann jakobhellermann deleted the push-mokuookxrmnu branch February 1, 2025 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants