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

Update "Getting Started" and software architecture docs #3313

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

williamckha
Copy link
Contributor

@williamckha williamckha commented Sep 8, 2024

Description

Testing Done

Resolved Issues

Resolves #1854
Resolves #3312
Resolves #3126

Length Justification and Key Files to Review

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

  • Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
  • Remove all commented out code
  • Remove extra print statements: for example, those just used for testing
  • Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

@williamckha williamckha added the Documentation Issues involving documentation label Sep 8, 2024
Comment on lines 78 to 82
5. Clone your fork of the repository. We recommend cloning using HTTPS since it is the easiest to set up. You should be able to clone your fork using the following command (you can put it wherever you like):
```
git clone https://github.com/<your_username>/Software.git
```
You can find your fork's remote URL under the green `Code` button on the main page of your fork on GitHub, under the HTTPS tab.
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't show HTTPS cloning. You won't be able to push/pull because github doesn't do password authentication anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well... I use https and it's easy peasy. But I forgot that I have a credential helper (GitHub CLI). Once you run gh auth login GitHub opens up in your browser and prompts you to sign in with 2fa. Then your credentials are cached and push/pull via https never prompts you for credentials

Copy link
Contributor

@itsarune itsarune Sep 18, 2024

Choose a reason for hiding this comment

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

https://docs.github.com/en/get-started/getting-started-with-git/about-remote-repositories#cloning-with-https-urls

yes, you need to either set up:

  1. personal access tokens (mayhaps too complicated for new members)
  2. Or set up a credential helper. Then, we should mention git credential manager as an additional required download.

Considering all of this together, I think SSH should still be our recommendation but we can mention HTTPS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, we can keep ssh as the default. I was just thinking of suggesting https since setting up ssh is not exactly straightforward either

Comment on lines 266 to 274
```cpp
def executeStrategy(IntentCoroutine::push_type& yield, Pass pass) {
do {
yield(/* align the robot to make the pass */)
}while(current_time < pass.start_time);
} while(current_time < pass.start_time);
do {
yield(/* kick the ball at the pass location */)
}while(/* robot has not kicked the ball */)
} while(/* robot has not kicked the ball */)
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit weird for us to show example code that doesn't follow our style guidelines

Copy link
Contributor

@itsarune itsarune left a comment

Choose a reason for hiding this comment

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

2 minor nits

Copy link

New page

Copy link
Contributor

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale Inactive pull requests label Oct 26, 2024
@williamckha williamckha removed the Stale Inactive pull requests label Oct 26, 2024
docs/getting-started.md Outdated Show resolved Hide resolved
docs/getting-started.md Outdated Show resolved Hide resolved
scripts/lint_and_format.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@itsarune itsarune left a comment

Choose a reason for hiding this comment

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

left some feedback

@williamckha
Copy link
Contributor Author

Made changes to our software architecture doc:

  • Rewrote Strategy / STP sections
  • Replaced Navigation section with newly written Motion Planning section
  • Got rid of some outdated sections (e.g. Intents)
  • Moved around some sections so that the information is ordered more sensibly IMO

I'd like to update/add diagrams at some point but I think I will do that in a different PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues involving documentation
Projects
None yet
2 participants