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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add more details about date parsing in the stats command #579

Merged
merged 2 commits into from
Oct 21, 2022

Conversation

bvergnaud
Copy link
Contributor

@bvergnaud bvergnaud commented Oct 21, 2022

Hello 馃憢馃徎

I just spent a bit of time figuring out what the stats command can do and what the proper syntax is. I figured I'd spare some people some searching in the future by updating the documentation.

  • Document the use of chrono_english and point to its documentation for date formatting.
  • Document the fact that one can manipulate a 24h window, rather that just a specific day.
  • Document the relationship with the dialect option in the configuration.
  • Update the existing examples and add a few more.

Also, I think the stats command's help is a bit misleading for 2 reasons:

  • The argument name period seems like a misnomer. Based on the code, the period is fixed, it's either 1-day or all of history. What the users really give is a starting point for that 1-day period.
  • period is represented as a list. I get the idea, treat the command's arguments as a vector and reconstruct the string from there so that the users don't need to quote the period. But it makes it look as though we could potentially give multiple periods, which does not work.

I haven't changed anything in that regard in this PR. I have my idea about a better way to deal with this but I have no experience with rust at all which would make implementing it slow and complicated. Maybe those problems are worth an issue, I'll wait for your feedback on that.

Have a good day ! :)

Copy link
Collaborator

@conradludgate conradludgate left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

I've been meaning to do it for a while, but I wrote a fork that is faster and less prone to panicking than chrono-english is: https://crates.io/crates/interim

Since this is related, can I ask you to change our use of chrono-english to interim with the chrono feature?

@conradludgate
Copy link
Collaborator

The argument name period seems like a misnomer. Based on the code, the period is fixed, it's either 1-day or all of history. What the users really give is a starting point for that 1-day period.

Good point. I'd be open to fixing this in an update. I'm not too concerned about this command regarding breaking changes. Wdyt @ellie

period is represented as a list. I get the idea, treat the command's arguments as a vector and reconstruct the string from there so that the users don't need to quote the period. But it makes it look as though we could potentially give multiple periods, which does not work

I think if we fix this, we should have start_date and an optional end_date. The implicit behaviour could remain a single day, but we can then provide the end date to make it longer. The example commands would change accordingly to atuin stats 'last friday'

conradludgate
conradludgate previously approved these changes Oct 21, 2022
@@ -55,6 +55,7 @@ impl FilterMode {
}

// FIXME: Can use upstream Dialect enum if https://github.com/stevedonovan/chrono-english/pull/16 is merged
// FIXME: Above PR was merged, but dependency was changed to interim (fork of chrono-english) in the ... interim
Copy link
Collaborator

Choose a reason for hiding this comment

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

:D

Copy link
Collaborator

Choose a reason for hiding this comment

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

This actually is still necessary because of the serde usage. The chrono-english PR doesn't add this serde feature so it's not enough. However I have ways to avoid a new type, which I can do some point later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I just remove both comments, then ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah it's amusing, keep it

@conradludgate conradludgate merged commit 8b9aae7 into atuinsh:main Oct 21, 2022
ellie pushed a commit that referenced this pull request Nov 4, 2022
* docs: add more details about date parsing in the stats command

* chore: Replace chrono-english crate with interim
@ellie ellie mentioned this pull request Nov 6, 2022
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.

None yet

2 participants