- 
                Notifications
    You must be signed in to change notification settings 
- Fork 11
Dev install #83
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
base: main
Are you sure you want to change the base?
Dev install #83
Conversation
….{namespace, dependencies}
    | next regexs but tmrw | 
There was a problem hiding this 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_findandtry_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- PackageSpecand- 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.)
| 
 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 | 
| I have removed  | 
There was a problem hiding this 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}")] | 
There was a problem hiding this comment.
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)
| 
 I didn't! I'll make unit-tests to make things easier to test | 
| I'll try to fix all bugs mentioned before merging too | 
…gets rid of a warning: `unused manifest key: bin.0.build`
Fix some minor things on the `dev-install` branch
This PR addresses
installcommand and workflow improvements.Changes
Workflow Updates:
Command Restructuring:
workspacecommand toproject(withprjalias)synccommand functionalityCode Quality:
f.len() > 0with!f.is_empty()for better readabilityDocumentation:
workspace→project)Build Configuration:
Tests:
Breaking Changes
workspacecommand is nowproject(prjfor short)add,deleteandbulk_deletecommands