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

add --reverse to history list #1252

Merged
merged 8 commits into from
Sep 29, 2023
Merged

add --reverse to history list #1252

merged 8 commits into from
Sep 29, 2023

Conversation

kiran-4444
Copy link
Contributor

Tried working on this issue: #1239
This is a WIP and a very minimal (and brute force) implementation I did to get a hang of the repo. Seeking feedback on this. Thanks!

@vercel
Copy link

vercel bot commented Sep 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atuin-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 29, 2023 11:00am

@deicon
Copy link
Contributor

deicon commented Sep 25, 2023

First off, please change this to be a Draft PR.

@kiran-4444 kiran-4444 marked this pull request as draft September 25, 2023 08:06
atuin/src/command/client/history.rs Outdated Show resolved Hide resolved
atuin/src/command/client/history.rs Outdated Show resolved Hide resolved
@deicon
Copy link
Contributor

deicon commented Sep 25, 2023

Documentation is missing for the new parameter
https://github.com/atuinsh/atuin/blob/main/docs/docs/commands/list.md

@deicon
Copy link
Contributor

deicon commented Sep 25, 2023

One last question here would be, if it would be better to get the result sorted from the DB instead of reverse the vec afterwards. I would argue, in this case here, reverse should be fine, but obviously not the most optimal way of doing things.

@deicon
Copy link
Contributor

deicon commented Sep 25, 2023

I asked for Draft, as it was titled WIP. But it looks like its more than WIP.

@kiran-4444
Copy link
Contributor Author

One last question here would be, if it would be better to get the result sorted from the DB instead of reverse the vec afterwards. I would argue, in this case here, reverse should be fine, but obviously not the most optimal way of doing things.

I'm intimidated to change that Database's list function, I'll now try to do this at DB level.

@deicon
Copy link
Contributor

deicon commented Sep 25, 2023

One last question here would be, if it would be better to get the result sorted from the DB instead of reverse the vec afterwards. I would argue, in this case here, reverse should be fine, but obviously not the most optimal way of doing things.

I'm intimidated to change that Database's list function, I'll now try to do this at DB level.

@ellie / @conradludgate probably have an opinion here.

@conradludgate
Copy link
Collaborator

One last question here would be, if it would be better to get the result sorted from the DB instead of reverse the vec afterwards. I would argue, in this case here, reverse should be fine, but obviously not the most optimal way of doing things.

We can avoid any reversing in sql or in code.

In print_list we iterate over the history items already in reverse.

for h in h.iter().rev() {

We can choose whether we want to iterate over h.iter() or h.iter().rev() depending.

Let me know if you want some more help navigating the rust side of implementing this

@kiran-4444
Copy link
Contributor Author

Let me know if you want some more help navigating the rust side of implementing this

Yeah, I need some help with that.

@deicon
Copy link
Contributor

deicon commented Sep 29, 2023

@kiran-4444 you might want to consoder to remove the WIP and make it an active PR

@kiran-4444 kiran-4444 changed the title wip: add --reverse to history list add --reverse to history list Sep 29, 2023
@deicon
Copy link
Contributor

deicon commented Sep 29, 2023

If no longer wip just mark as active :)

@kiran-4444 kiran-4444 marked this pull request as ready for review September 29, 2023 10:19
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.

Thanks :)

@conradludgate conradludgate merged commit 5044006 into atuinsh:main Sep 29, 2023
9 checks passed
@kiran-4444
Copy link
Contributor Author

kiran-4444 commented Sep 29, 2023

Are there any issues I can work on? Went through issues list... but I just some help in selecting one.

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