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

added some explanation for usage and a commenting etiquette section. #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dheadrick1618
Copy link
Member

No description provided.

@@ -1,5 +1,26 @@
# obc_software_nextgen_test
Next generation linux based OBC software
Fprime software project to be ran on an embedded linux OS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should 'ran' be 'run'? (caveat: I'm no English expert)

@@ -18,8 +39,14 @@ The following is a list of possible branch types:
- documentation

### Branching etiquette
- Branches must implement/change only a single feature.Changes related to that feature across different layers of code is acceptable.
- Branches must implement/change only a single feature. Changes related to that feature across different layers of code is acceptable.
- You may create branches off branches, but they will be merged in the order they were created. You also need to be prepared to accept the risk of having to rebase to a new parent branch parent branch, should those changes be declined.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 'parent branch' is repeated
More importantly, it may not be possible to guarantee that branches are merged in chronological order, which you point out in this sentence. It might be easier to require that branches are independent, i.e. the merge order doesn't matter. This may also make it easier to enforce a best practice: that no single branch (PR) can break the build or tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we not be able to enforce that branches are merged in chronological order? I agree with making branches independent to prevent from breaking builds and tests, but how would we handle the situation where branches are created off other branches. Or would this just not be allowed?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason you need to create a branch from a branch is because the new branch has a dependency on the underlying branch. That means that you likely can't make a PR from the new branch, because it will break the build until the underlying branch is merged. What usually happens is you make the PR for the new branch once the underlying branch PR is merged. So it's not that branches off branches aren't allowed, it's that breaking the build is not allowed.

@@ -18,8 +39,14 @@ The following is a list of possible branch types:
- documentation

### Branching etiquette
- Branches must implement/change only a single feature.Changes related to that feature across different layers of code is acceptable.
- Branches must implement/change only a single feature. Changes related to that feature across different layers of code is acceptable.
- You may create branches off branches, but they will be merged in the order they were created. You also need to be prepared to accept the risk of having to rebase to a new parent branch parent branch, should those changes be declined.
- If you discover a bug unrelated to the feature you are working on, switch back to master, make a branch to fix the feature, then make a PR to merge that Bugfix. Chances are you aren't the only one experiencing the bug. You may rebase your development branch off the Bugfix branch.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: make a branch to fix the 'bug', not 'feature'

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.

2 participants