Skip to content

Make code organization match the functional structure #139

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

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Sep 12, 2024

part of #132

We are moving dft so there is an engine and 2 uis (the terminal tui and the cli)

Let's make the code structure match this functional structure:

  1. Rename ui to tui to make it clear it is the terminal ui
  2. Move the CLI related code into its own module (cli)
  3. Move the command line parsing structures into DftArgs in in main.rs

I don't fully understand the split between what logic belongs in app and what belongs in tui but I figure we can work on that split over time

@@ -35,7 +35,7 @@ use tonic::transport::Channel;

use crate::{
app::{state::tabs::history::HistoryQuery, AppEvent},
ui::SelectedTab,
tui::SelectedTab,
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 renamed ui to tui

Copy link
Collaborator

Choose a reason for hiding this comment

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

i wont have time for a full review right now, but as a drive by comment i think what would align more to the structure i have in mind is renaming app to tui. The ui code is actual display code for the tui app (i.e. it is ratatui widgets like Table, Textarea, Paragraph, etc.). i can imagine having a directory structure something like this:

tui/state
tui/handlers
tui/ui
cli/...
execution/...
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call -- done in 9ea59d6

@@ -88,15 +80,14 @@ pub struct App<'app> {
}

impl<'app> App<'app> {
pub fn new(state: state::AppState<'app>, cli: DftCli) -> Self {
pub fn new(state: state::AppState<'app>) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cli arguments don't appear to be used after construction so I removed them from the structures

@@ -329,118 +320,3 @@ pub async fn run_app(cli: cli::DftCli, state: state::AppState<'_>) -> Result<()>
}
app.exit()
}

/// Encapsulates the command line interface
pub struct CliApp {
Copy link
Contributor Author

@alamb alamb Sep 12, 2024

Choose a reason for hiding this comment

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

moved into cli


use clap::Parser;
Copy link
Contributor Author

@alamb alamb Sep 12, 2024

Choose a reason for hiding this comment

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

I moved the CLI parsing into main.rs

Update: moved into args.rs

@alamb alamb marked this pull request as ready for review September 12, 2024 20:46
src/main.rs Outdated
}

Ok(())
}

const LONG_ABOUT: &str = "
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesnt need to be in this PR but maybe we put all of the clap logic in something like args.rs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 5e88d0a

@alamb
Copy link
Contributor Author

alamb commented Sep 13, 2024

I had to get the PR checked out to fix the formatting, so I went ahead and made the changes you suggested

While I was at it, I also noticed that the config is not really part of the terminal ui so I moved it from app/config.rs to config.rs

@matthewmturner matthewmturner merged commit b5183ad into datafusion-contrib:main Sep 13, 2024
3 checks passed
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