Skip to content

Feature csv import #2669

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

wyattearp
Copy link

this provides a simple csv file import. after reviewing #2556 and #2431, it never seemed like there was an answer to this, so here's my take at a first cut. i will apologize in advance that i'm not super familiar with rust, so feedback is appreciated.

in keeping with the way other parts of the tool operate, we look for a history.csv file with the format of:

timestamp,duration,command
1613322469,10,cargo install atuin
..., ..., ...

the test provides an example. note that the class/module is called csv_importer, CsvHistroyImporter because I ran into conflicts with csv and the crate it belongs to, happy to take suggestions.

Checks

  • I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • I have checked that there are no existing pull requests for the same thing

@wyattearp
Copy link
Author

for what it's worth, i've tested this as "works for me":

wyatt@lazarus ~/git_repos/atuin (feature-csv-import) $ cat target/release/history.csv 
timestamp,duration,command
112345,10,ls -als
223453,5,echo "hi"
wyatt@lazarus ~/git_repos/atuin (feature-csv-import) $ atuin history list | head -n 3
1970-01-02 03:12:25	ls -als	10s
1970-01-03 10:04:13	echo "hi"	5s
2024-04-03 20:43:52	ls	0s

Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

hey! thank you for the PR - couple of comments

#[derive(Debug)]
pub struct CsvHistoryImporter;

fn default_histpath() -> Result<PathBuf> {
Copy link
Member

Choose a reason for hiding this comment

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

The default_histpath approach made sense for classic shell history import where there is usually an expected location for data, but I think in this case it isn't needed

Instead, we should make it so that the import csv subcommand takes a mandatory argument of the file to import

Copy link
Author

Choose a reason for hiding this comment

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

there's not a facility i'm seeing in any of the other importers to specify the location of a file as part of the command line parsing - am i missing it somewhere?

Copy link
Author

Choose a reason for hiding this comment

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

trying to dig more on this (excuse my rust n00bness) - it looks like it would be either a modification to the main Importer class, which means everything would get an option, but only one of them would support it, or i create a single "one off" function to basically mimic async fn import but takes a path.

are there better options i'm not seeing?

@@ -26,6 +27,8 @@ pub enum Cmd {
ZshHistDb,
/// Import history from the bash history file
Bash,
/// Import history from user csv history file
CsvHistoryImporter,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CsvHistoryImporter,
Csv,

we have atuin import bash, atuin import zsh etc, so atuin import zsh makes more sense here

Copy link
Author

@wyattearp wyattearp Apr 11, 2025

Choose a reason for hiding this comment

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

updated

@@ -111,6 +114,7 @@ impl Cmd {
Self::Zsh => import::<Zsh, DB>(db).await,
Self::ZshHistDb => import::<ZshHistDb, DB>(db).await,
Self::Bash => import::<Bash, DB>(db).await,
Self::CsvHistoryImporter => import::<CsvHistoryImporter, DB>(db).await,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Self::CsvHistoryImporter => import::<CsvHistoryImporter, DB>(db).await,
Self::Csv => import::<CsvHistoryImporter, DB>(db).await,

see rename change

Copy link
Author

@wyattearp wyattearp Apr 11, 2025

Choose a reason for hiding this comment

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

updated

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.

2 participants