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

[Discussion] Restructure Command #26

Closed
TornaxO7 opened this issue Oct 5, 2022 · 6 comments
Closed

[Discussion] Restructure Command #26

TornaxO7 opened this issue Oct 5, 2022 · 6 comments

Comments

@TornaxO7
Copy link
Contributor

TornaxO7 commented Oct 5, 2022

TL;DR

I propose to write every command as a struct instead of putting all commands into an enum. What do you think about @AethanFoot

Motivation

A command has a lot of properties in my opinion. This includes:
- matching a string to themself
- converting themself to a string
- do that what their name says (like reload the daemon if the Reload Command was called)
- having different arguments

those are all I could find yet and in my opinion this is why we should split the commands up from an enum into a struct for each command.

How should it look like

Example:
config/command/mod.rs

mod chord;
mod execute;
mod exit_chord;
mod kill;
mod reload;

use std::convert::TryFrom;
use crate::errors::LeftError;

pub use self::{chord::Chord, execute::Execute, exit_chord::ExitChord, reload::Reload};

pub trait Command<'a>: ToString + Default + TryFrom<&'a str> {}

config/command/chord.rs

use crate::config::Keybind;

use super::Command;

#[derive(Debug, PartialEq)]
pub struct Chord(Vec<Keybind>);

impl Chord {
    pub fn new(keybinds: Vec<Keybind>) -> Self {
        Self(keybinds)
    }
}

impl<'a> Command<'a> for Chord {
    // bla bla
}

// other functions which are *only* relevant to `Chord`

Pros and Cons

Pros

  • logic of each command is only isolated in a file, while an enum gets bigger
    and bigger and the the enum includes the whole logic of all enum-entries

yeah, that's basically it. All functions which affect to all commands (like
searching through them) can be put to config/command/mod.rs, the "main"
command file.

Cons

  • the syntax might be a little bit strange for the beginning like
    here.
    This should be, in my opinion:
fn main() {
    // ...
    send_command(command::Kill::boxed());
    // ...
}

fn send_command(command: Box<dyn Command>) {
    let path = errors::exit_on_error!(BaseDirectories::with_prefix(lefthk_core::LEFTHK_DIR_NAME));
    let pipe_name = Pipe::pipe_name();
    let pipe_file = errors::exit_on_error!(path.place_runtime_file(pipe_name));
    let mut pipe = fs::OpenOptions::new().write(true).open(&pipe_file).unwrap();
  
    // difference is here with `command.to_string()`
    writeln!(pipe, "{}", command.to_string()).unwrap();
}

which would be more typesafety and also makes it easier to change the code since
you don't have to replace the string "Kill" everywhere if you want to rename
this command.

@TornaxO7
Copy link
Contributor Author

TornaxO7 commented Oct 6, 2022

@AethanFoot (ping)

@AethanFoot
Copy link
Member

This seems reasonable. Do you mean for this to be used in both places the enums currently are, as my only worry would be the config side in lefthk being overly complicated but internally in lefthk-core this would be fine.

@TornaxO7
Copy link
Contributor Author

TornaxO7 commented Oct 6, 2022

Maybe we could introduce something like an Adapter-Trait which converts a self-defined-command struct into a lefthk-core-command

@AethanFoot
Copy link
Member

Yeah that seems okay

@TornaxO7
Copy link
Contributor Author

TornaxO7 commented Oct 6, 2022

alright, then I'd like to take care of this :)

@TornaxO7
Copy link
Contributor Author

Done by #27

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

No branches or pull requests

2 participants