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

Implement option for printing custom formats #1043

Merged
merged 1 commit into from
May 8, 2024

Conversation

tmccombs
Copy link
Collaborator

@tmccombs tmccombs commented Jun 16, 2022

This is mostly a proof of concept before merging there is still some work that needs to be done for:

  • Write new tests (both unit tests and integration tests)
  • Add documentation to man page
  • Potentially add support for outputting arbitrary bytes on unix like we do for normal printing (I'm not going to worry about this for now)
  • Benchmark, and optimize if necessary
    • one potential optimizaiton could be to write directly to the stream rather than building an intermediary OsString, I started looking into that, but it got kind of messy.

Also, this doesn't currently support colorized output when using format string. I think that for an initial implementation that is probably fine.

@sharkdp
Copy link
Owner

sharkdp commented Jul 11, 2022

Thank you very much for taking a stab at this!

I didn't have time to properly review it, but I think I am in favor of integrating this new option! That is, unless it leads to a performance penalty for the default case (without --format).

Something that I thought about in a different context, but seems fitting here: I'm not a huge fan of the {}/{/}/{//}/{.}/… syntax, which we borrowed from GNU parallel, I believe. Would it make sense to think about something a bit more verbatim? I think we could easily support something like {path}/{basename}/{parent}/{without_extension}/… in addition. What do you think? I'd much rather type out those words than having to look up the syntax each time.

@tmccombs
Copy link
Collaborator Author

I like the more verbose names too.

@sharkdp
Copy link
Owner

sharkdp commented Jul 12, 2022

👍. We can also do this in a follow up PR though.

@sharkdp
Copy link
Owner

sharkdp commented Sep 11, 2022

Regardless of the short/long syntax discussion, I'd like to see this being integrated to fd!

@tmccombs Are you still interested in finishing this? It would probably be best to focus on the benchmarks first, to make sure that we don't run into performance regressions.

@tmccombs
Copy link
Collaborator Author

Sorry it took me so long to get back to this. I am interested in finishing this. Though I wouldn't mind some help with benchmarking it.

@sharkdp
Copy link
Owner

sharkdp commented Dec 8, 2023

It would be great to pick this up again. Anything I can do? I can definitely take care of the benchmarks. I could also take a look at fixing the merge conflicts (?).

@tmccombs
Copy link
Collaborator Author

tmccombs commented Dec 9, 2023

I fixed the merge conflicts.

One question I have is if we would rather have a --printf option with more format optiions, such as access and modified times, owning user and group, etc. Certainly not as extensive as find, but more than just variants of the path. Although, I really don't love the idea of having two different format syntaxes.

@sharkdp
Copy link
Owner

sharkdp commented Dec 13, 2023

One question I have is if we would rather have a --printf option with more format optiions, such as access and modified times, owning user and group, etc. Certainly not as extensive as find, but more than just variants of the path. Although, I really don't love the idea of having two different format syntaxes.

I would personally like if we extend the {…} syntax. For example, we could have more descriptive aliases like {basename} instead of {/}. This would allow us to use something like {access_time} and similar if we want to add more fields, without having to come up with a cryptic/concise syntax. If we need more control over how a field like a date is printed, we could follow Python f-strings syntax with something like {access_time:%Y-%m-%d}. I don't think that we need to have a find -printf compatibility mode here. What do you think?

@tmccombs
Copy link
Collaborator Author

That sounds good to me.

@cohml
Copy link

cohml commented Jan 21, 2024

@tmccombs @sharkdp - Any chance this will be merged soon?

I'm dying for this feature and compulsively checking this PR every few days as a result. Please put me out of my misery lol.

long,
value_name = "fmt",
help = "Print results according to template",
conflicts_with = "list_details"
Copy link
Owner

Choose a reason for hiding this comment

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

👍

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you very much and sorry for the late review!

@tmccombs
Copy link
Collaborator Author

tmccombs commented May 8, 2024

I've run a few benchmarks with hyperfine, and haven't noticed a significant difference in performance.

@tmccombs tmccombs merged commit ea22cbd into sharkdp:master May 8, 2024
18 checks passed
@tmccombs tmccombs deleted the format-option branch May 8, 2024 05:10
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