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

Add more information about core team processes #20720

Merged
merged 1 commit into from
Mar 27, 2025

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Mar 4, 2025

No description provided.

@wouterj wouterj requested a review from fabpot March 4, 2025 21:06
@carsonbot carsonbot added this to the 6.4 milestone Mar 4, 2025
@wouterj wouterj force-pushed the contributing-docs branch 2 times, most recently from 4d0ae9d to 632493a Compare March 4, 2025 21:14
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

Thanks for adding such informations, that's really helpful!

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

Wouter, thanks a lot for contributing these improvements. Thanks to reviewers too.


I like everything except this change:

"minor change" --> "peripheral change"

Minor is self-explanatory and everybody is used to it. Peripheral is an euphemism that needs to be explained in order to know exactly what it refers to. I don't think "minor" is offensive or problematic at all. Some changes, some work, some ideas, etc. are minor compared to others. I think that's fine.

@wouterj
Copy link
Member Author

wouterj commented Mar 7, 2025

Minor is self-explanatory and everybody is used to it. Peripheral is an euphemism that needs to be explained in order to know exactly what it refers to. I don't think "minor" is offensive or problematic at all. Some changes, some work, some ideas, etc. are minor compared to others. I think that's fine.

See #20720 (comment). This doesn't have anything to do with offensive wording, but rather using 2 separate definitions of "minor change". One is when you should merge a PR as minor/bug/feature, and the one I changed to "peripheral" is what is referred to as typo fixes/cs fixes that core team members are allowed to push to the repository directly (e.g. after a merge).

@fabpot
Copy link
Member

fabpot commented Mar 7, 2025

Minor is self-explanatory and everybody is used to it. Peripheral is an euphemism that needs to be explained in order to know exactly what it refers to. I don't think "minor" is offensive or problematic at all. Some changes, some work, some ideas, etc. are minor compared to others. I think that's fine.

See #20720 (comment). This doesn't have anything to do with offensive wording, but rather using 2 separate definitions of "minor change". One is when you should merge a PR as minor/bug/feature, and the one I changed to "peripheral" is what is referred to as typo fixes/cs fixes that core team members are allowed to push to the repository directly (e.g. after a merge).

Maybe "insignificant" changes would do? Or "unsubtantial"?

@javiereguiluz
Copy link
Member

I disagree about the need to split the current "minor change" concept into two concepts: minor and peripheral/insignificant.

I think we need simpler processes and rules, instead of making them more complicated.

@wouterj
Copy link
Member Author

wouterj commented Mar 10, 2025

We don't change any rules, the document as it currently is live uses "minor change" for 2 different definitions. This is just to remove confusion by making sure each term only has 1 definition.

@javiereguiluz
Copy link
Member

javiereguiluz commented Mar 10, 2025

That's where we disagree. The current document only has one type of minor change that comprises this:

Minor changes comprise typos, DocBlock fixes, code standards
violations, and minor CSS, JavaScript and HTML modifications.

This has served us well for so many years that, in my opinion, we shouldn't make any change about it.

@wouterj
Copy link
Member Author

wouterj commented Mar 10, 2025

We have two:

image

The first one allows more changes than the second one. Currently, the document can confuse new team members into thinking that only PRs containing exclusively "typos, CS, docblock fixes, minor CSS/JS/HTML modification" should be merged as "minor". Which is not the case and results in a more convoluted changelog. This is why I think it's important to disconnect the two of them.

@javiereguiluz
Copy link
Member

Ah, I see my misunderstanding now. Thanks 🙏

I consider both the same. To me, all the things listed in both cases are "minor things that don't need to be listed in the CHANGELOG file":

  • Typos
  • Changes in Markdown files
  • Adding test files
  • Adding or updating translations
  • Docblock fixes
  • Code syntax fixes
  • CSS/JS/HTML updates

@wouterj
Copy link
Member Author

wouterj commented Mar 10, 2025

But I don't think we want core team members to push PHP logic changing commits to the version branches without PR, which would be the case if we combined it into one definition.

@wouterj wouterj force-pushed the contributing-docs branch from 75c0c32 to 7e8256c Compare March 22, 2025 21:51
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Once comments resolved. Thank you!

@wouterj wouterj force-pushed the contributing-docs branch 2 times, most recently from 78d8efd to de32673 Compare March 27, 2025 22:11
@wouterj wouterj force-pushed the contributing-docs branch from de32673 to d6b4ff9 Compare March 27, 2025 22:12
@wouterj wouterj merged commit abd43fb into symfony:6.4 Mar 27, 2025
3 checks passed
#. A clone for their own contributions, which they use to push to their
fork on GitHub. Clear out the push URL for the Symfony repository using
``git remote set-url --push origin dev://null`` (change ``origin``
to the Git remote poiting to the Symfony repository);
Copy link
Member

Choose a reason for hiding this comment

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

typo: pointing :)

@wouterj wouterj deleted the contributing-docs branch March 28, 2025 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants