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

Rewrite contributing guide #83

Merged
merged 3 commits into from Nov 26, 2021
Merged

Conversation

fredpi
Copy link
Member

@fredpi fredpi commented Nov 24, 2021

The (hopefully) last PR before the 3.0 release.

Addresses #81.

The changelog file currently doesn't meet the requirements mentioned in the new contributing guide, but I will update it before releasing version 3.0.

@StevenSorial
Copy link
Member

@fredpi Thanks for the PR.

Branch names

Please use the work/* branch naming scheme for any changes and write meaningful commit messages (see rationale here).

I'm not too convinced by this. I rarely see this convention being used even in very large repositories. Even the mentioned article recommends using user\somebranch which makes more sense, if all PRs are going to be under work/somebranch, then the word 'work' becomes redundant and meaningless.
This is just a comment, tbh I don't care about branch names (for PRs), since they are going to be deleted when the PR is merged anyways.

Merges

I would like that we recommend that merges have to be squashed to one commit, it will clear the clutter and the back-and-forth that happens in the PR discussions.

Adding new symbols

Should we mention the generation of the restriction messages, since it's the only part that is not generated by make? I don't know if it makes sense to mention it in the guide since it's relatively complicated.

I don't have much experience in writing guides, so I hope @knothed will be more helpful than me :)

@knothed
Copy link
Collaborator

knothed commented Nov 25, 2021

@fredpi @Stevenmagdy keep up the great work! I’m really excited to see the 3.0 release.

Regarding the work/* branch naming: I‘m on the same page as @Stevenmagdy here, I don’t like that scheme either.

This project is intended to be a safe, welcoming space for collaboration, and contributors are expected to adhere to the Contributor Covenant code of conduct.

Is this section really required? I think it’s just pointing out the obvious. Why would contributors be toxic in the first place – and if they were, would this note really stop them from being so?

I’m fine with the rest.

@fredpi
Copy link
Member Author

fredpi commented Nov 26, 2021

@Stevenmagdy @knothed Thanks for your reviews!

I'm not too convinced by this. I rarely see this convention being used even in very large repositories. Even the mentioned article recommends using user\somebranch which makes more sense, if all PRs are going to be under work/somebranch, then the word 'work' becomes redundant and meaningless. This is just a comment, tbh I don't care about branch names (for PRs), since they are going to be deleted when the PR is merged anyways.

This branch naming scheme comes from a variant of the Git Flow structure, but I understand that it should not really be mandatory. I will therefore remove it from the contributing guide.

I would like that we recommend that merges have to be squashed to one commit, it will clear the clutter and the back-and-forth that happens in the PR discussions.

I have some reservations here. Even after a PR is merged, it is sometimes useful to have a history of things that happened in the development process. E. g. if we would want to change the documentation style merged in #82 to a style that has previously been implemented and committed during the development of #82, it would be useful to access previous commits. After all, having small, separated commits is one of the purposes of Git.

I only wonder whether multiple commits, each with large changes to the generated code, may cause a performance hit when interacting with the Git repo (compared to the case that there's only one single squashed commit).

What do you think? @Stevenmagdy @knothed I suggest we let the majority of us "vote" on this issue. If the performance hit is negligible, I'd be in favor of non-squashed merge commits.

Should we mention the generation of the restriction messages, since it's the only part that is not generated by make? I don't know if it makes sense to mention it in the guide since it's relatively complicated.

I think this part is too complicated to be put into the guide – if it's really needed, it should rather be described in the header comments of the respective file.

This project is intended to be a safe, welcoming space for collaboration, and contributors are expected to adhere to the Contributor Covenant code of conduct.

Is this section really required? I think it’s just pointing out the obvious. Why would contributors be toxic in the first place – and if they were, would this note really stop them from being so?

Yeah, I think it is required. It is pointing out the obvious and may not stop people from being toxic, but nonetheless it's important IMHO to have a clearly specified set of rules everyone is obliged to obey to (and otherwise face consequences like being blocked from interaction with this repo).

@knothed
Copy link
Collaborator

knothed commented Nov 26, 2021

Okay, I'm happy with these suggestions.

Regarding squashing commits: I don't really care – both ways have their benefits. I hardly believe there will be a performance hit, so that shouldn't be an argument in favor of squashing.
As long as the number of new MRs per time stays manageable, I wouldn't be in favor of requiring to squash.

@fredpi fredpi merged commit f68ed95 into stable Nov 26, 2021
@fredpi fredpi deleted the work/rewrite-contributing-guide branch November 26, 2021 15:22
@fredpi fredpi mentioned this pull request Nov 26, 2021
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.

None yet

3 participants