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

feat: init git repo #65

Merged
merged 4 commits into from
Mar 21, 2024
Merged

feat: init git repo #65

merged 4 commits into from
Mar 21, 2024

Conversation

CinematicCow
Copy link
Contributor

This PR solves for #61. It initializes a new git repo with initialized parachain as the commit message. It takes the default global user config from git and uses it as the signature.

Currently the test passes even if there is no .git dir in the new parachain. I need help with that.

@weezy20
Copy link
Contributor

weezy20 commented Mar 17, 2024

@AlexD10S can you review?

@Daanvdplas
Copy link
Collaborator

Hello @CinematicCow! Amazing that you are contributing to our project.

Currently the test passes even if there is no .git dir in the new parachain. I need help with that.

That is because it is initialised at line 34 and 68.

This is repetitive code and we should use your helper function. If you could remove the ones mentioned above and expand your test by including the situation of no git config and other test cases it would be amazing!

@AlexD10S
Copy link
Collaborator

Thanks for contributing into the project. As Daan said it looks like is initialize in another function in the create new parachain process.

Regarding to @weezy20 question, if no name or email the following line will throw an error:

let signature = repo.signature()?;

We want to display an error message to the user in that case?

The rest of the code looks good to me, my only question is why we need the commit?

let commit_id = repo.commit(Some("HEAD"), &signature, &signature, message, &tree, &[])?;

@weezy20
Copy link
Contributor

weezy20 commented Mar 18, 2024

@AlexD10S The commit is required so that the parachain that's created by pop new is ready for manipulation by pop add

@CinematicCow
Copy link
Contributor Author

  • Fixed the test. The test now asserts on the number of commits.
  • Signature missing error is now handled and displayed to the user.

Any ideas on how to handle the test on CI?

@AlexD10S AlexD10S self-requested a review March 19, 2024 06:57
AlexD10S
AlexD10S previously approved these changes Mar 19, 2024
weezy20
weezy20 previously approved these changes Mar 19, 2024
src/commands/new/parachain.rs Outdated Show resolved Hide resolved
@CinematicCow CinematicCow dismissed stale reviews from weezy20 and AlexD10S via ed8ebb9 March 21, 2024 11:46
@weezy20 weezy20 merged commit 4327abd into r0gue-io:main Mar 21, 2024
1 check passed
@weezy20
Copy link
Contributor

weezy20 commented Mar 21, 2024

Thank you for your contributions @CinematicCow. This is much needed to make add-pallet work seamlessly. Enjoy using pop!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants