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

Docs/contrib updates #1736

Merged
merged 6 commits into from
Dec 8, 2021

Conversation

brenthuisman
Copy link
Contributor

  • Add documentation for collaboration on a PR using just git
  • Add the stage of Issue creation to PR workflow.
  • Correct header level in test docs

@thorstenhater thorstenhater self-assigned this Dec 7, 2021
Copy link
Contributor

@thorstenhater thorstenhater left a comment

Choose a reason for hiding this comment

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

Some stylistic issues and typos plus a few suggestions on clarity.

Issues
------

New features, bugfixes or other kinds of contributions ideally start their lives as an Issue on our
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue -> issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although a bit subtle, I capitalize when I'm referring to things in GH UI. You might have many issues, but until they are Issues, I probably don't know about them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea, but maybe highlight in a more obvious way then and explain it. Like written, it seems like a typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See below.


New features, bugfixes or other kinds of contributions ideally start their lives as an Issue on our
`Issue tracker <https://github.com/arbor-sim/arbor/issues>`_. Having an Issue before an implementation
addressing the Issue (code contribution or otherwise) gives others the chance to weigh in and help
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have explained that: replace Isssue with request here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, maybe to subtly worded right now, but I want to stress that the thing being addressed exists as a Github Issue. Referring to them as requests is a layer of indirection that I would rather leave out, unless you have a strong argument for doing that anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

The sentence as stated

Having an Issue before an implementation addressing the Issue

is syntactically dubious -- at least. Without having an Issue one cannot address it.
Either the first or second Issue should refer to something different. I think you
mean this

Make a (formal GH-kind) Issue before addressing your (desire-for-change-kind) issue (in code/text).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated with a definition (like there already was for PR) the first time the term is used.

Copy link
Contributor

@thorstenhater thorstenhater left a comment

Choose a reason for hiding this comment

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

I'll accept despite informalisms. They might justified here due to the less formal tone of the section.

@brenthuisman brenthuisman merged commit e2ecd6a into arbor-sim:master Dec 8, 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.

2 participants