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

fix #18 #23

Merged
merged 1 commit into from
Oct 23, 2024
Merged

fix #18 #23

merged 1 commit into from
Oct 23, 2024

Conversation

VladimirFokow
Copy link
Collaborator

@VladimirFokow VladimirFokow commented Oct 23, 2024

integrates the @mitkonikov's solution, in issue #18

@VladimirFokow VladimirFokow merged commit f3ace8d into Manim-Notebook:main Oct 23, 2024
@Splines
Copy link
Member

Splines commented Oct 23, 2024

So we don't want to do PR reviews anymore?

@VladimirFokow
Copy link
Collaborator Author

VladimirFokow commented Oct 23, 2024

my bad, sorry

What should we do before merging each PR?
I don't have experience.

For example, is at least 2 reviews required?
Is it possible to make it a hard requirement in GitHub? Should we?

@Splines
Copy link
Member

Splines commented Oct 23, 2024

Is it possible to make it a hard requirement in GitHub? Should we?

Yes, it's possible to define branch protection rules in GitHub and I think we definitely should, @bhoov. Even for small changes, it's worth to have another pair of eyes look over code.

Maybe something like at least one review required? Or if you want to look over every PR then you can also define that at least one review is required from the author.

@bhoov
Copy link
Collaborator

bhoov commented Oct 23, 2024

Last night i made you both "collaborators" on this repo when I realized I could not request either of you to be "reviewers" on my own PR. Also, between my job, school, and an 18 month old I'm barely keeping up with both of your paces of development 🙈.

I am happy with a rule that says we need at least 1 other reviewer for each PR. I do not need to be that reviewer, but I will try to monitor all PRs in these early stages of development.

Additionally, what do you think of the following plan:

  1. Protecting the main branch. This will be our primary branch for development
  2. Making a new deploy branch that will contain the state of the code that is currently deployed to the marketplace

@Splines
Copy link
Member

Splines commented Oct 23, 2024

Also, between my job, school, and an 18 month old I'm barely keeping up with both of your paces of development 🙈.

Yeah, totally understandable. In the long-term I will also not be able to keep up this pace, this is probably just in the beginning phase until we reach a point that is somewhat stable. And even for now, I'm in uni and each week might be different with regards to how much time I can spare for a bit of development on the side. I mean, no ones pays us to do this ;)

Additionally, what do you think of the following plan:

I like the plan of having separate branches. Just out of convention in other projects, what do you think about using a main and a dev branch instead? Where the main branch is tagged and each tag corresponds to one release on the marketplace. It doesn't have any advantages (maybe just a tiny one that main is usually the default branch and therefore when viewing the repo, you see production-ready code and not dev code. But that's really not too important).

Then, from time to time we can do a Continuous Release 0.x, which is just a PR from dev to main. So

  • squash and merge from feature/my-branch or fix/my-branch to dev
  • merge commit from dev to main and after that deploy to marketplace. At MaMpf, we've even set it up such that each tag creation on main triggers a webhook to our server and there it automatically redeploys the website.

Note that at MaMpf, I've also aggregated some conventions that we use for Git in a wiki entry. E.g. it also includes the convention for commit messages (which I use for PR titles as well, e.g. capitalize the start, not too long, in imperative form).

@Splines
Copy link
Member

Splines commented Oct 23, 2024

Also note that with us being collaborators now, @VladimirFokow, we can directly create branches on this repo (such that I don't have to add remote repos/forks to my git remote -v list to checkout the 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
Development

Successfully merging this pull request may close these issues.

3 participants