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

[Search history] Make date time format configurable #299

Merged
merged 5 commits into from
Jun 26, 2023
Merged

[Search history] Make date time format configurable #299

merged 5 commits into from
Jun 26, 2023

Conversation

tamirzb
Copy link
Contributor

@tamirzb tamirzb commented Jun 25, 2023

Enable configuring the format of command execution times in Search History through the new fzf_history_time_format variable. This is highly valuable for non-US users who may be accustomed to reading dates as day-month (rather than month-day as the default is), or for people who want to search through previous years of command history.

@@ -20,7 +24,7 @@ function _fzf_search_history --description "Search command history. Replace the
$fzf_history_opts |
string split0 |
# remove timestamps from commands selected
string replace --regex '^\d\d-\d\d \d\d:\d\d:\d\d │ ' ''
string replace --regex '^.* │ ' ''
Copy link
Owner

Choose a reason for hiding this comment

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

Is this greedy? Cause if so and the command itself contains a |, then it'll consume too much

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I tested this with commands that had │and it worked correctly. Only way this can go wrong is if the user puts │ in their fzf_history_time_format, but I considered it a pretty far fetched edge case so didn't handle it.

If you prefer, I could add a check to make sure there is no │ in the user's fzf_history_time_format.

Alternatively, if you want to feel a bit safer about the regex, I can change the * to [^│].

Copy link
Owner

Choose a reason for hiding this comment

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

If you prefer, I could add a check to make sure there is no │ in the user's fzf_history_time_format.

Let's just put that in the docs. Can you take a stab at updating the readme? And I'll polish it for you.

No, I tested this with commands that had │and it worked correctly.

Great, but now I'm confused why it works...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok actually you are right, my bad, the regex is greedy. I will fix it. And yes I will also update the readme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fixed the code and updated the README. Go ahead and polish it :)

@tamirzb
Copy link
Contributor Author

tamirzb commented Jun 25, 2023

Update: I realized that I didn't include the regex for the preview window, so fixed that.

--preview-window="bottom:3:wrap" \
$fzf_history_opts |
string split0 |
# remove timestamps from commands selected
string replace --regex '^\d\d-\d\d \d\d:\d\d:\d\d │ ' ''
string replace --regex '^[^│]* │ ' ''
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind just doing the non-greedy version of what you had? I think it's .*?. I think it's more idiomatic and slightly easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized I was wrong and it didn't work properly. When I had inside a command in my history it removed also the beginning of the command.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah but that's probably because you didn't make it non-greedy. That's what the ? does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok sure, changed it.

The user can configure the time format by setting the
fzf_history_time_format variable
@tamirzb tamirzb requested a review from PatrickF1 June 26, 2023 06:58
@PatrickF1
Copy link
Owner

Nice, looks good! I'll polish the readme tomorrow. Btw, what's your use case for configuring the date time format?

@tamirzb
Copy link
Contributor Author

tamirzb commented Jun 26, 2023

I guess two things: First, my history spans over multiple years, so when I'm searching for things I can reach commands from different years, so having the year in there makes it much more insightful. Second, I am not American, so having the month before the day confuses me 😅

@PatrickF1
Copy link
Owner

I guess two things: First, my history spans over multiple years, so when I'm searching for things I can reach commands from different years, so having the year in there makes it much more insightful.

Great use case!

Second, I am not American, so having the month before the day confuses me 😅

Oh my gosh me and my American ignorance! Please excuse me. Could you explain what language you speak primarily and what dates in that language looks like?

@PatrickF1 PatrickF1 changed the title [Search history] Add an option to configure the time format [Search history] Make date time format configurable Jun 26, 2023
@tamirzb
Copy link
Contributor Author

tamirzb commented Jun 26, 2023

Oh my gosh me and my American ignorance! Please excuse me. Could you explain what language you speak primarily and what dates in that language looks like?

Hahaha no worries. And to be honest I don't think it's a uniqueness of where I'm from, rather of the US. As you can see on Wikipedia, most of the world uses day-month-year as their primary date format, and month-day-year is pretty rare.

Anyway, as long as it's customizable it's really no big deal :)

@roland-5
Copy link
Collaborator

You both sucks. The best format is year-month-day. :P I must say, it's nice feature. I don't have that much history, but there was few times when I struggle searched something similar within years.

@PatrickF1
Copy link
Owner

Oh my gosh me and my American ignorance! Please excuse me. Could you explain what language you speak primarily and what dates in that language looks like?

Hahaha no worries. And to be honest I don't think it's a uniqueness of where I'm from, rather of the US. As you can see on Wikipedia, most of the world uses day-month-year as their primary date format, and month-day-year is pretty rare.

Anyway, as long as it's customizable it's really no big deal :)

Haha I totally forgot about that. What's worse is we're still not on the metric system 😭. Thanks for your input! I'll merge this soon.

@PatrickF1 PatrickF1 merged commit db5a67a into PatrickF1:main Jun 26, 2023
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