linutil icon indicating copy to clipboard operation
linutil copied to clipboard

refactor: Split linutil into TUI and Core crates

Open lj3954 opened this issue 1 year ago • 5 comments

Pull Request

Title

Split linutil into TUI and Core crates

Type of Change

  • [x] Refactoring

Description

Restructure linutil from one crate containing all logic and scripts into two crates, along with a separate tabs directory to contain all data previously housed in src/commands/. Temporary directories are also entirely handled within the core crate, since they're not used anywhere else (since #153, script paths have been absolute).

Testing

Linutil TUI builds and functions correctly. The core crate is correctly rebuilt upon any modification to the tabs directory.

Impact

Future contributions should be simplified, since the project is structured more clearly. Additional UIs can be added (e.g. #248) without binary bloat or build flags that heavily complicate the codebase.

Despite the appearance of large changes, current PRs should be mostly compatible with this. The only major changes are to Cargo.toml files, most of everything else is just moved to other directories.

  • Closes #272 (includes modified opt-level)

Checklist

  • [x] My code adheres to the coding and style guidelines of the project.
  • [x] I have performed a self-review of my own code.
  • [x] I have commented my code, particularly in hard-to-understand areas.
  • [x] My changes generate no errors/warnings/merge conflicts.

lj3954 avatar Sep 06 '24 00:09 lj3954

Aren't there gonna be github workflows issues here?

adamperkowski avatar Sep 11 '24 12:09 adamperkowski

No. There is still only 1 binary target.

lj3954 avatar Sep 11 '24 14:09 lj3954

Yes I see.

I'd suggest renaming directories for:

  • core to linutil_core
  • tui to linutil_tui

(https://rust-lang.github.io/api-guidelines/naming.html)

and removing edition = "2021" from Cargo.tomls for the crates (replacing with edition.workspace = true) and adding it to the main one under [workspace.package].

Other than that, your PR is a very good idea. If you ask me, @ChrisTitusTech, I'd try to resolve everything and merge this ASAP.

adamperkowski avatar Sep 11 '24 15:09 adamperkowski

  • core to linutil_core

  • tui to linutil_tui

This is in the linutil repository, it's redundant to include it in the directory name. The crate names should include linutil since they could be published on crates.io or another similar repository. The guidelines you linked do not specify anything about matching the directory & crate's names.

and removing edition = "2021" from Cargo.tomls for the crates (replacing with edition.workspace = true) and adding it to the main one under [workspace.package].

I thought about that for a while, but I decided against it. The TUI will be far more actively maintained, and updating it to take advantage of features in new Rust editions would be more convenient if you don't have to also update completely functional code in another crate. Similarly, dependencies, even if overlap is present, aren't all being shared between the 2 crates unless is completely necessary.

lj3954 avatar Sep 11 '24 22:09 lj3954

This is in the linutil repository, it's redundant to include it in the directory name. The crate names should include linutil since they could be published on crates.io or another similar repository. The guidelines you linked do not specify anything about matching the directory & crate's names.

If linutil is going to be published on crates.io (#333), there would be two separate crates (your PR) and linutil_tui would depend on linhtil_core from crates.io. I think it's better to rename the directories to avoid confusion.

The TUI will be far more actively maintained, and updating it to take advantage of features in new Rust editions would be more convenient if you don't have to also update completely functional code in another crate.

Agreed. This makes sense.

adamperkowski avatar Sep 11 '24 23:09 adamperkowski