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

refactor: Split linutil into TUI and Core crates #274

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

lj3954
Copy link
Contributor

@lj3954 lj3954 commented Sep 6, 2024

Pull Request

Title

Split linutil into TUI and Core crates

Type of Change

  • 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.

Checklist

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

@lj3954 lj3954 force-pushed the restructure_linutil branch 2 times, most recently from 32585a4 to 0280041 Compare September 6, 2024 06:30
@lj3954 lj3954 force-pushed the restructure_linutil branch from 0280041 to 8786959 Compare September 6, 2024 06:37
Copy link
Owner

@ChrisTitusTech ChrisTitusTech left a comment

Choose a reason for hiding this comment

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

Love the changes but I need to work through some of the PRs as this will break them.

@adamperkowski
Copy link
Collaborator

Aren't there gonna be github workflows issues here?

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 11, 2024

No. There is still only 1 binary target.

@adamperkowski
Copy link
Collaborator

adamperkowski commented Sep 11, 2024

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.

@lj3954
Copy link
Contributor Author

lj3954 commented Sep 11, 2024

  • 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.

@adamperkowski
Copy link
Collaborator

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.

Copy link
Owner

@ChrisTitusTech ChrisTitusTech left a comment

Choose a reason for hiding this comment

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

This is awesome, a way better structure for future development!

@ChrisTitusTech ChrisTitusTech merged commit 7104991 into ChrisTitusTech:main Sep 12, 2024
1 of 2 checks passed
@lj3954 lj3954 deleted the restructure_linutil branch September 12, 2024 22:51
@ChrisTitusTech ChrisTitusTech added the enhancement New feature or request label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants