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

chore(docs): extends docs for contributors #3760

Merged

Conversation

MateuszTrN
Copy link
Contributor

Adds guidelines to the docs

Description

as disscused with @nikolasrieble here: https://recharts.slack.com/archives/C042Q5K5UDC/p1694458300038969?thread_ts=1693662193.441469&cid=C042Q5K5UDC

added two bullet points int the docs

Related Issue

#3407

Motivation and Context

Unifies and standardize branching names

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

CONTRIBUTING.md Outdated
@@ -48,7 +48,7 @@ _Before_ submitting a pull request, please make sure the following is done…

- Search [GitHub](https://github.com/recharts/recharts/pulls) for an open or closed Pull Request that relates to your submission. You don't want to duplicate effort.

- Fork the repo and create your branch from `master`.
- Fork the repo and create your branch from `master` using the branch naming convention: `(bugfix|feature|chore)/i(#ISSUE_ID)-branch-name`. Example: `bugfix/i(#9999)-fixes-very-important-issue`
Copy link
Contributor

Choose a reason for hiding this comment

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

What would you say is the main value from having such a suggestion?
I am open to it, but want to give room for discussion the pros and cons.

Alternatively to having a convention for the branch name, it might make sense to instead have a convention for the PR name, as we squash and merge by default, where the PR name becomes the commit message on master.

I personally prefer a rule for the PR title, given the context.

Either way, if we do care about this, we might want to setup a GHA to validate this rules.

Curious to hear other opinions and some motivation here.

Copy link
Member

Choose a reason for hiding this comment

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

Since we squash and merge I agree @nikolasrieble, you could branch off master and PR for all it matters to me 😅.

The only thing I generally follow is conventional commit https://www.conventionalcommits.org/en/v1.0.0/#summary messages as a personal preference.

A general PR name format sounds fine to me, but I don't really expect people to follow it unless right there is some sort of automation. We can update the PR template (which we need to do anyways) to add that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
I feel like I made a fool of myself a bit :D
tl;dr; yeah this change has no sense

My thought process I had two reasons:
I like to keep branch names clean and conventional, cause I use sourceTree as a git tool, and I like have my branches in folders - which sourcetree does which such convention (separates bugs, chores and features) - and it is helpful when more people work on a repo to have it clean.
And it would have been right, if it hadn't been for those meddling forks!
HOWEVER - I didn't put a thought on it, that the way ppl contribute to this repo is by forking so everyone has very limited amount of branches in their repos and they might not care for any of this, which kills my point - I still can keep my fork as I please.

But my main reason, that I also have just learnt is not valid, was in the issue page, right pane. It has this "Development" section - which I am used to have branches (now it has only open PRs, if PR is closed it gets empty again - usually my team use Altassian suite, so I'm used to that it always auto-magically links everything if the issue ID is present anywhere) - but I've just realized that here, issue page does not care for forked repos (which is good, I just fell for force of habits and wanted to have there "on going work" - It's just my first time contributing on github so I didn't think it through).

To sum up - I should write this in the "Motivation" section in the PR - but the point was:
Improve 'Development' section in the right pane of the issue page, which I've learnt, is naive of me to think my suggestion would help.

edit: but to be honest I still don't understand why this PR is not shown in this section of the #3407 issue

Copy link
Contributor

Choose a reason for hiding this comment

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

@MateuszTrN I still believe this PR to be valuable, as it allows for us to discuss and align this specific topic. A PR is always a great way to get things going, because it is specific. Thats why I appreciate your contribution 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.

thanks!
I reverted changes to the branching part. I already applied your suggestion regarding stories.

MateuszTrN and others added 2 commits September 16, 2023 11:49
chore(docs): applies code review suggestion

Co-authored-by: Nikolas Rieble <nikolas-rieble@protonmail.com>
chore(docs): reverts changes after discussion
Copy link
Member

@ckifer ckifer left a comment

Choose a reason for hiding this comment

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

Thanks!!

@ckifer ckifer merged commit b7ed8d5 into recharts:master Sep 17, 2023
5 checks passed
GMer78 pushed a commit to GMer78/recharts-1 that referenced this pull request Nov 24, 2023
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.

None yet

3 participants