-
Notifications
You must be signed in to change notification settings - Fork 49
feat: init new git repo for new projects #558
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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.
I added a few comments to refine messages and comments.
Let me know what do you think 😉
(The code review will still be performed by @Valerioageno since I'm not a rust expert 😅)
Thank you! What do you think of my proposed logic for the git workflow (description in PR overview)? |
Please can I have a bit of help here? When I use I could ignore the dead code warning, but that feels bad. WDYT? |
@Valerioageno should I just remove some of these tests or comment them out/disable them so they can be rewritten once we have a mock? |
Right now the tests are not reliable enough to be merged since they strongly rely on the host machine that run them (in a machine that doesn't have installed I think that for this iteration we could:
WDYT? |
Should we put this on hold until we get the mock (I'm happy to help with that if needed), and then merge those changes into this branch and refactor the tests as needed? I think it's a bit risky to merge this one without tests. |
Makes sense! The goal with the other PR is to find a library that supports configuration at build time. When we have that we can proceed with this then! :) |
This PR is unblocked now! |
So to be clear, the only thing left to do is write some kind of mock for git? |
Exactly, otherwise the test is dependant on the software installed on the running machine. |
Let me know @jacobhq if you need support with this! :) |
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.
I'd easier down it even more.
Let's remove the --git-init
flag (all the installations also init git if existing).
This way we can also skip the check is_git_installed
and simply run the command at the end of the routine.
Let's then remove the error handling in init_new_git_repo
and simply check trace every error with a meaningful message.
WDYT?
Oh ok, so just run the command and use the trace macro if it fails? |
Yeah, I'd keep it super minimal |
Checklist
Related issue
Fixes #554
Overview
Checks if git is installed, and init a new repo by default. If git is not installed, we continue in silence, and only fail with error if the user specifies the git flag, but git is not installed.
Tasks
Add flag to docs (not much to do, but will update the help snippet on the cli page)