Skip to content

Conversation

@Thumuss
Copy link
Member

@Thumuss Thumuss commented Aug 17, 2025

This PR addresses install command and workflow improvements.

Changes

Workflow Updates:

  • Updated Rust toolchain from 1.85 to 1.89 in CI workflows
  • Re-enabled clippy, rustfmt, and doc generation checks

Command Restructuring:

  • Renamed workspace command to project (with prj alias)
  • Enhanced sync command functionality

Code Quality:

  • Replaced f.len() > 0 with !f.is_empty() for better readability
  • Improved error handling and validation logic
  • Removed unused imports and cleaned up dependencies

Documentation:

  • Updated README to reflect command changes (workspaceproject)
  • Updated feature descriptions and examples

Build Configuration:

  • Simplified Cargo.toml feature flags
  • Removed justfile configuration

Tests:

  • Added unit-testing

Breaking Changes

  • The workspace command is now project (prj for short)
  • Removed add, delete and bulk_delete commands

@Thumuss
Copy link
Member Author

Thumuss commented Aug 17, 2025

next regexs but tmrw

Copy link
Contributor

@SillyFreak SillyFreak left a comment

Choose a reason for hiding this comment

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

Apart from the comments on individual code pieces, this is what stood out to me while looking through the changes:

commit messages: the term "refactor" only applies for changes that don't affect functionality, not to e.g. "install working", "remove bulk_delete"

in general, is it useful to have features for every individual command? If we want them: i think the code for individual features should be better isolated. E.g. the regexes in utils.rs all have their own #[cfg(feature = "...")]. if a regex is really command specific, I think it's better to declare it in the command's module.

In general, prefer functions over macros. using two different functions if there are different arguments is preferred in Rust.

  • good macro uses: utpm_bail (does control flow), utpm_log (useful variadics for formatting)
  • not so good: write_manifest/read_manifest (could be two functions each, where the one delegates to the other by specifying the default value, e.g. try_find and try_fin_in)
  • format_package: it makes sense that there are multiple options, but I feel this could be better represented by implementing a trait on PackageSpec and VersionlessPackageSpec, as well as maybe on a new struct Namespace(pub String);

none of these things are meant as things to be fixed before this PR can be merged, just as things to consider in the future. (And considering the comments doesn't mean they need to be followed in every situation either, of course.)

@Thumuss Thumuss mentioned this pull request Aug 19, 2025
@Thumuss
Copy link
Member Author

Thumuss commented Aug 20, 2025

  • format_package: it makes sense that there are multiple options, but I feel this could be better represented by implementing a trait on PackageSpec and VersionlessPackageSpec, as well as maybe on a new struct Namespace(pub String);

Looks like a bit complicated to impl... I use an other way to impl this on other command. I think I'll stick to that for now since I can't find a way I like

@Thumuss
Copy link
Member Author

Thumuss commented Aug 20, 2025

I have removed format_package! but I didn't create a trait, as I didn't find a real use-case other than one command.
I will have that in mind tho

Copy link
Contributor

@SillyFreak SillyFreak left a comment

Choose a reason for hiding this comment

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

Assuming you have tested this, the latest changes look good. in state.rs there are many empty lines introduced; I commented on one example

/// A git-related error.
#[cfg(any(feature = "install", feature = "clone", feature = "publish"))]
#[error("Git error: {0}")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the #[cfg] line should have been removed, not blanked (many other occurrences in this file)

@Thumuss
Copy link
Member Author

Thumuss commented Aug 20, 2025

Assuming you have tested this [...]

I didn't! I'll make unit-tests to make things easier to test

@Thumuss
Copy link
Member Author

Thumuss commented Aug 21, 2025

I'll try to fix all bugs mentioned before merging too
Testing is on his way too

Fix some minor things on the `dev-install` branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants