refactor example isn't a refactor #88

New Issue

GiteaMirror · 2026-02-17T11:44:29-06:00

GiteaMirror commented

2026-02-17 11:44:29 -06:00

Originally created by @gabrielf on GitHub (Jan 16, 2020).

refactor!: drop support for Node 6

BREAKING CHANGE: refactor to use JavaScript features not available in Node 6.

But is this really a refactor? I guess people have slightly different definitions of refactoring but Wikipedia says (emphasis mine):

Code refactoring is the process of restructuring existing computer code—changing the factoring—without changing its external behavior

With that definition you cannot have a refactor commit that also contains a breaking change.

I think the example should be changed to something like:

refactor: extract foo function to reduce duplication

The example containing ! could be changed to a feat:.

Originally created by @gabrielf on GitHub (Jan 16, 2020). The [example of a refactor](https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with-both-and-breaking-change-footer) is > refactor!: drop support for Node 6 > > BREAKING CHANGE: refactor to use JavaScript features not available in Node 6. But is this really a refactor? I guess people have slightly different definitions of refactoring but Wikipedia says (emphasis mine): > Code refactoring is the process of restructuring existing computer code—changing the factoring—**without changing its external behavior** With that definition you cannot have a refactor commit that also contains a breaking change. I think the example should be changed to something like: > refactor: extract foo function to reduce duplication The example containing `!` could be changed to a `feat:`.

GiteaMirror closed this issue

2026-02-17 11:44:29 -06:00

GiteaMirror commented

2026-02-17 11:44:31 -06:00

@stevemao commented on GitHub (Jan 16, 2020):

I agree this is not the best example. And previously you couldn't include breaking changes in refactor but we loosen the rule a bit. Maybe we could write a better example of refactor and use either feat or some custom tag to demonstrate !

@stevemao commented on GitHub (Jan 16, 2020): I agree this is not the best example. And previously you couldn't include breaking changes in refactor but we loosen the rule a bit. Maybe we could write a better example of refactor and use either `feat` or some custom tag to demonstrate `!`

GiteaMirror commented

2026-02-17 11:44:32 -06:00

@shaman-apprentice commented on GitHub (Mar 8, 2021):

And previously you couldn't include breaking changes in refactor but we loosen the rule a bit

May I ask, why did you loosen it? From my point of view it should definitely not be allowed, as a refactor prohibits breaking changes by definition.

@shaman-apprentice commented on GitHub (Mar 8, 2021): > And previously you couldn't include breaking changes in refactor but we loosen the rule a bit May I ask, why did you loosen it? From my point of view it should definitely not be allowed, as a refactor prohibits breaking changes by definition.

GiteaMirror commented

2026-02-17 11:44:33 -06:00

@damianopetrungaro commented on GitHub (Mar 8, 2021):

@gabrielf good one tho!
Wanna open a PR using feat in the examples you pointed out?

@damianopetrungaro commented on GitHub (Mar 8, 2021): @gabrielf good one tho! Wanna open a PR using `feat` in the examples you pointed out?

GiteaMirror commented

2026-02-17 11:44:34 -06:00

@shaman-apprentice commented on GitHub (Oct 8, 2021):

@gabrielf I hope you don't mind I created a PR with my ideas.

Changing to "feat!: drop support for Node 6" didn't feel right to me, as dropping something is not really a new feature, is it? So I came up with "feat!: enforce authentication for all requests".

I also was so free to remove the example with both "!" and "BREAKING CHHANGE" footer. I think commit messages shouldn't contain duplicate information. Also I felt it is less to read for the reader of conventionalcommits.org, which makes it a very little more attractive for potentially new adopters.

Feel very welcome to let me know, what you think about it :)

@shaman-apprentice commented on GitHub (Oct 8, 2021): @gabrielf I hope you don't mind I created a PR with my ideas. Changing to "feat!: drop support for Node 6" didn't feel right to me, as dropping something is not really a new feature, is it? So I came up with "feat!: enforce authentication for all requests". I also was so free to remove the example with both "!" and "BREAKING CHHANGE" footer. I think commit messages shouldn't contain duplicate information. Also I felt it is less to read for the reader of conventionalcommits.org, which makes it a very little more attractive for potentially new adopters. Feel very welcome to let me know, what you think about it :)

GiteaMirror commented

2026-02-17 11:44:35 -06:00

@javier-godoy commented on GitHub (Jul 12, 2022):

I agree that the new example is easier to understand, however:

@gabrielf

Code refactoring is the process of restructuring existing computer code—changing the factoring—without changing its external behavior

@shaman-apprentice

A refactor prohibits breaking changes by definition.

How would you call a change that renames a function? Assume that the function is part of the public API, thus renaming it is a breaking change. It doesn't change its "external behavior" either (unless the function name is considered part an attribute of the behavior).

@javier-godoy commented on GitHub (Jul 12, 2022): I agree that the new example is easier to understand, however: @gabrielf > Code refactoring is the process of restructuring existing computer code—changing the factoring—without changing its external behavior @shaman-apprentice > A refactor prohibits breaking changes by definition. How would you call a change that renames a function? Assume that the function is part of the public API, thus renaming it is a breaking change. It doesn't change its "external behavior" either (unless the function name is considered part an attribute of the behavior).

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/conventionalcommits.org#88