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

feature: Exposes isExecutable to the user #3153

Closed
wants to merge 4 commits into from

Conversation

arran4
Copy link
Contributor

@arran4 arran4 commented Aug 5, 2023

Exposes isExecutable to the user. I'm not sure if this is consistent as there are other functions that should be exported, and fileExists is expected to be done with stat from what I can tell.

Copy link
Collaborator

@halostatue halostatue left a comment

Choose a reason for hiding this comment

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

This also looks good to me, although I still think that findExecutableIn needle haystack would be Good To Have, although it becomes a fairly simple extension over the exposed isExecutable template function.

@arran4
Copy link
Contributor Author

arran4 commented Aug 6, 2023

I did some testing, it is possible for us to bridge: https://pkg.go.dev/golang.org/x/exp/slices into it. Given: golang/go#57433

arran4 and others added 2 commits August 6, 2023 11:55
…ing if it's an executable

BREAKING CHANGE: `isExecutable` no longer always returns false and tries to determine execution status by file extension

```
{{ if isExecutable "/bin/echo" }}
`sh` is executable
Copy link
Owner

Choose a reason for hiding this comment

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

This should be echo is executable. You should also remove the backticks around sh as this is text or shell, not Markdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're correct on both accounts. I would fix it but I think I missed the boat on that.

@twpayne
Copy link
Owner

twpayne commented Aug 6, 2023

I did some testing, it is possible for us to bridge: https://pkg.go.dev/golang.org/x/exp/slices into it. Given: golang/go#57433

I don't think this is possible as text/template does not support functions as values (assuming you were expecting to use slices.ContainsFunc).

@twpayne
Copy link
Owner

twpayne commented Aug 6, 2023

Superseded by #3157.

@arran4
Copy link
Contributor Author

arran4 commented Aug 6, 2023

I have a sample project with it where I'm able to do that. I haven't extensively tested it. But I got it working. However I suspect it will have significant issues with generics until the base libraries such as text/template are updated to support it. (I am assuming they have not been.)

Was the reason for re-authoring the commit in a new PR for speed and to keep it to a single commit?

@twpayne
Copy link
Owner

twpayne commented Aug 6, 2023

Was the reason for re-authoring the commit in a new PR for speed and to keep it to a single commit?

Mostly speed. I have some vacation coming up and want to get a new version of chezmoi out before I leave. You've been very responsive, but it's also been quite a lot of discussion over the last few days and we're in very different timezones.

@arran4
Copy link
Contributor Author

arran4 commented Aug 6, 2023

Having issues sleeping helped with the responsiveness and timezone not with the attention to detail in communication or code.

Enjoy your vacation. If I put through another PR I'm not expecting a speedy response.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants