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 CONTRIBUTING.md #1311

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

Conversation

playfulFence
Copy link
Contributor

closes #1225

@playfulFence playfulFence added the skip-changelog No changelog modification needed label Mar 19, 2024
@playfulFence playfulFence self-assigned this Mar 19, 2024
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Looking pretty good from my perspective, but I think we should revert inlining the links.

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

Looks good overall, just left a few comments. Regarding the -ing suggestions, they are just for consistency with other headers, feel free to change it the other way.

CONTRIBUTING.md Outdated
Comment on lines 16 to 17
* [Your Pull Request: From Submission to Merge](#your-pull-request-from-submission-to-merge)
* [Community and Communication](#community-and-communication)
Copy link
Member

Choose a reason for hiding this comment

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

These two items are not working or not present, not sure if we need a toc, when viewing a readme in gh you already have the outline:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO:

  1. having Quick Navigation part in text is a bit more reader-friendly and beautiful 😄
  2. "Outline" function is rarely used, from my experience

However, if you insist and think it's a critical point - will revert it back

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@jessebraham
Copy link
Member

(Sorry I'm reading some material relating to contributing guides, will do a proper review within the next few business days)

@jessebraham jessebraham marked this pull request as draft April 11, 2024 13:33
@jessebraham
Copy link
Member

Sorry for not getting to this, had a lot on my plate.

I think we should meet as a team to discuss how best to move forward with this. I think there is a fair bit of information that should ultimately be included in CONTRBUTING.md (and/or other documents), so it'd be nice to come up with a bit of a game plan first.

CC @MabezDev

@MabezDev
Copy link
Member

Now that we have the xtask, I think we should also document some do's and don't of how to use it. For example we should state that all xtask commands should be run from the root of the project, not in a sub project.

@playfulFence
Copy link
Contributor Author

playfulFence commented Apr 27, 2024

@MabezDev, agree!
Maybe let's discuss this topic on a next meeting(/when most of us won't have a vacation 😄)? Just to put this matter to rest.

cc: @jessebraham (as you've had some thoughts and suggestions on this)

upd: Also worth mentioning is the update to the HIL tests in case related API was changed

@bjoernQ bjoernQ mentioned this pull request May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the CONTRIBUTING.md guide
4 participants