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

FR: be able to set commit_timestamp contextually #5366

Open
avamsi opened this issue Jan 15, 2025 · 7 comments
Open

FR: be able to set commit_timestamp contextually #5366

avamsi opened this issue Jan 15, 2025 · 7 comments
Labels
enhancement New feature or request

Comments

@avamsi
Copy link
Member

avamsi commented Jan 15, 2025

I prefer author timestamp to committer timestamp in general but found it to be somewhat useless for evolog and working copy (it'll probably be confusing at times to have working copy use committer timestamp and all other commits use author timestamp but I want to try).

Given evolog mostly has hidden commits, I tried something like:

	'commit_timestamp(commit)' = '''
		if(commit.current_working_copy() || commit.hidden(),
			commit.committer().timestamp().ago(),
			commit.author().timestamp().ago()
		)
	'''

.. which doesn't work because I'm setting commit_timestamp to a template, not to a timestamp.


This example is perhaps not the strongest case for it, but it would be nice for templates to able to resolve / assert to specific types. That aside, I'm curious if there's any other way to do what I want (other than changing code to provide more hooks, I guess).

@yuja
Copy link
Contributor

yuja commented Jan 15, 2025

This is a limitation of the current template engine, which can't express generics internally. If we rewrite the engine in a way that template values are dynamically typed, maybe we can change if(x, y) to return the common type of x and y.

For now, we'll probably need format_commit_timestamp(commit).

@yuja yuja added the enhancement New feature or request label Jan 15, 2025
@jennings
Copy link
Contributor

jennings commented Jan 15, 2025

commit_timestamp(commit) should return a timestamp, not a formatted string, so in the template you posted, you should remove the .ago() calls:

	'commit_timestamp(commit)' = '''
		if(commit.current_working_copy() || commit.hidden(),
			commit.committer().timestamp(),
			commit.author().timestamp()
		)
	'''

However, maybe we should have something like a latent context that we can use? Spitballing:

	'commit_timestamp(commit)' = '''
		if(context.command() == "evolog",
			commit.committer().timestamp(),
			commit.author().timestamp()
		)
	'''

@yuja
Copy link
Contributor

yuja commented Jan 16, 2025

if() returns Template, so it wouldn't work anyway. Templater doesn't support generic if<T>(T, T) -> T nor if<T>(T) -> Option<T>.

if(context.command() == "evolog",

For this use case, #5311 will help.

@avamsi
Copy link
Member Author

avamsi commented Jan 16, 2025

you should remove the .ago() calls:

Yep! I did exactly that initially (and what Yuya said).

I was trying another hack and pasted that in the OP by mistake (I'll leave it as is now). So the hack is to use commit_timestamp like (yet to exist) format_commit_timestamp and set format_timestamp to identity function: 'format_timestamp(timestamp)' = 'timestamp' (of course, "timestamp" is a template here, not a timestamp).

@yuja
Copy link
Contributor

yuja commented Jan 20, 2025

Templater doesn't support generic if<T>(T, T) -> T nor if<T>(T) -> Option<T>.

Btw, I think this can be implemented in the same way that we handle comparison operators, but we can't make if<T> be compatible with if<Template>.

In if<Template>: if(true, author.timestamp(), committer.timestamp()) returns a labeled template label("author timestamp", author.timestamp()). In if<T>: if(true, author.timestamp(), committer.timestamp()) would return author.timestamp() of Timestamp type. Auto-labeling context is lost. So we can't add a single if() function that falls back to Template depending on the arguments.

@avamsi
Copy link
Member Author

avamsi commented Jan 20, 2025

Yuya, I wonder if some sort of type assertions are easier to implement?
Asserting if<T1, T2> is of type T would only work if both T1 and T2 are of type T and so on..

@yuja
Copy link
Contributor

yuja commented Jan 20, 2025

I wonder if some sort of type assertions are easier to implement?

No. It would require rewrites. Templater internals are strictly typed. It's difficult to deal with dynamically-specified type names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants