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

thoughts on churn #22351

Closed
MylesBorins opened this issue Aug 16, 2018 · 6 comments
Closed

thoughts on churn #22351

MylesBorins opened this issue Aug 16, 2018 · 6 comments
Labels
discuss Issues opened for discussions and feedbacks. meta Issues and PRs related to the general management of the project.

Comments

@MylesBorins
Copy link
Contributor

hey all,

I was reading through the contribution guidelines for the git project today and noticed an interesting bit.

Fixing style violations while working on a real change as a
preparatory clean-up step is good, but otherwise avoid useless code
churn for the sake of conforming to the style.

While backporting from master to 8.x, 9.x, and 10.x I've noticed an increase in refactoring that is not supporting a more substantial change in the codebase. These changes have improved the developer experience of node, tightened our style guide, and have quite a number of people more involved in the project.

The churn does have a cost, 8.x in particular has been quite a bit harder to backport too. With 10.x moving to LTS soon and stumbling across the above bit from the git repo, I thought it might be a good idea to weigh the pro's an con's to such a policy and if it something the project may want to adopt.

To be explicit, I am torn. Very interested in other collaborators thoughts here.

@vsemozhetbyt vsemozhetbyt added discuss Issues opened for discussions and feedbacks. meta Issues and PRs related to the general management of the project. labels Aug 16, 2018
@gireeshpunathil
Copy link
Member

I echo your view, and share similar opinion. While style nits are good, flooding our project machinery with those can potntially cause:

  • features and bug fixes take a backseat
  • difficult to maintain
  • recursively introduce new bugs
  • strain our people
  • strain our CI

Probably recommending style nit PRs to only doc is a good idea?

@cjihrig
Copy link
Contributor

cjihrig commented Aug 16, 2018

I would love to restrict churn to docs and tests.

@jasnell
Copy link
Member

jasnell commented Aug 16, 2018

Assuming we can make progress on the notion of an automated commit queue, a great deal of churn can be dealt with more organically.

@mhdawson
Copy link
Member

@jasnell I'm not sure I understand your comment. I think Myles concern was from the additional work the churn introduces to the backporting process as opposed to the work to land the commits in the first place. I think the commit queue only addresses the latter.

@mhdawson
Copy link
Member

I'd also suggest that it could be worded to suggest limiting the amount of fixup is included in a PR for another purpose. If there are a larger number of changes for fixup its better that they be in a different PR.

@apapirovski
Copy link
Member

Given the lack of movement here since August, I'm going to close it out but feel free to reopen and continue the conversation. Just closing out stale issues with no clear action plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests

7 participants