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

Documentation about contributing to Stardis #247

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

RyanGroneck
Copy link
Contributor

📝 Description

Type: 📝 documentation

Adding documentation for guided instructions on setting up Stardis for would-be developers, and how to make your first Pull Request. This PR is still a work in progress until otherwise indicated.

Also, link issues affected by this pull request by using the keywords: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • My changes can't be tested (explain why)

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@RyanGroneck RyanGroneck marked this pull request as draft February 3, 2025 14:56
@RyanGroneck RyanGroneck changed the title contributing to Stardis documentation Documentation about contributing to Stardis Feb 3, 2025
@RyanGroneck
Copy link
Contributor Author

I built the new documentation locally, and everything seemed right and proper

@RyanGroneck RyanGroneck marked this pull request as ready for review February 3, 2025 19:34
@@ -0,0 +1,125 @@
{
Copy link
Contributor

@jvshields jvshields Feb 3, 2025

Choose a reason for hiding this comment

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

capitalization


Reply via ReviewNB

@@ -0,0 +1,159 @@
{
Copy link
Contributor

@jvshields jvshields Feb 3, 2025

Choose a reason for hiding this comment

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

Maybe make the title more specific

A First Timer's Example Guide to Contribution


Reply via ReviewNB

@@ -0,0 +1,159 @@
{
Copy link
Contributor

@jvshields jvshields Feb 3, 2025

Choose a reason for hiding this comment

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

Maybe a comment at the beginning saying, "This takes you through contributing using WSL and VScode with my personal installations."

Also mention that this assumes you have already forked stardis.


Reply via ReviewNB

@@ -0,0 +1,159 @@
{
Copy link
Contributor

@jvshields jvshields Feb 3, 2025

Choose a reason for hiding this comment

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

Maybe not bash specifically, maybe just say command line or terminal

Also you probably don't need to mention the OS.


Reply via ReviewNB

@@ -0,0 +1,159 @@
{
Copy link
Contributor

@jvshields jvshields Feb 3, 2025

Choose a reason for hiding this comment

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

Probably we don't need to complain about the standard name for every github repository main branch

"This way, when you make the Pull request to apply that change, the reviewers know exactly what your change does, and the proccess goes smoothly."

This is true but somewhat independent of making new branches for each feature. Probably instead you can talk about how this keeps things tidy and easier to review. When you open the PR we can focus on perfecting code that implements a single feature, or can revert a single commit later if it does something wrong.


Reply via ReviewNB

@@ -0,0 +1,159 @@
{
Copy link
Contributor

@jvshields jvshields Feb 3, 2025

Choose a reason for hiding this comment

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

hearts -> heart's

Careful about assuming the person is using specifically vscode on windows


Reply via ReviewNB

@@ -0,0 +1,159 @@
{
Copy link
Contributor

@jvshields jvshields Feb 3, 2025

Choose a reason for hiding this comment

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

git add -A adds all changed files to the PR. You might want to add that to the end of 1), and mention that you can also add individual files by name.

I'd also add a line here saying you can do git status to see which files are staged for for the commit, and which files are changed but are not part of it.

Also at the end of this whole notebook, I think it's worth mentioning that the vast majority of pull requests get feedback and are iterated upon before they're merged into the code, and to not be discouraged if people request changes. This is a normal part of the code development process.


Reply via ReviewNB

@jvshields
Copy link
Contributor

Having looked more, I do think removing the "starting from scratch" notebook from this PR still makes the most sense.

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