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

Should feat cover changes to existing features? #294

Open
stoically opened this issue Sep 11, 2020 · 12 comments
Open

Should feat cover changes to existing features? #294

stoically opened this issue Sep 11, 2020 · 12 comments

Comments

@stoically
Copy link

stoically commented Sep 11, 2020

Changing or dropping an existing feature (and as such changing the public API) is not covered by the core spec currently as far as I'm aware. This leads to confusion around what type to use in this case. Personally I like the definition "feat describes a change related to the public API"

I'd propose to change

The type feat MUST be used when a commit adds a new feature to your application or library.

into something that covers "changes or drops an existing feature"

@damianopetrungaro
Copy link
Member

@bcoe what do you think?

@bcoe
Copy link
Contributor

bcoe commented Sep 25, 2020

Let's start putting the meeting-agenda label on issues we think we'd like to discuss on a monthly call.

I opened an issue for us to have a first meeting -- my hope is that getting a few collaborators in a room together, we can better evolve the spec.

@wesleytodd
Copy link

I do think this description of feat is limited, but I think your above example has only one case not covered: dropping a feature. Changing is either a fix to the existing feature or a new feat on top of the existing feature.

Dropping a feature will always be a ! or BREAKING CHANGE, but it is IMO explicitly not when a commit adds a new feature to your application or library..

When I do this, I typically call it fix!: removed feat x because I always try to do a feat!: deprecate feat x first, and so the fix just removes the dep and method/property/behavior. I can see how this might not actually solve the problem for many, but I see a deprecation warning as a breaking feature, the following removal is also breaking but a fix since it is already deprecated.

Do you have any other prior art or approaches projects take for this kind of commit?

@stoically
Copy link
Author

stoically commented Sep 30, 2020

Personally I'd say that changing a feature is not a fix. In my mind a fix is a bug fix with the primary task to restore intended feature behavior, which might encompass changing a feature but probably shouldn't.

Yeah, modifying or dropping existing feature behavior is probably almost always breaking.

There's also changing an existing feature by adding new behavior in a backwards-compatible way. Depending on how the feat spec is interpreted, it could mean that it doesn't cover this, since "changing" / "modifying" / "extending" isn't mentioned.

@stoically
Copy link
Author

Do you have any other prior art or approaches projects take for this kind of commit?

Two random commits I just looked up:

@wesleytodd
Copy link

I am not sure your distinction matters to semver, tooling, or users reading the message. I am also not sure what type of change you can make which is not fixing a bug (restoring old behavior or fixing unintended behavior) and also not an new feature or sub-feature. If you have a concrete example that might help me understand.

There's also changing an existing feature by adding new behavior in a backwards-compatible way.

I think this fits well within the meaning of feat. Remember this is about "semverness", and to semver (and my interpretation) all of those are the same ("changing" / "modifying" / "extending").

Two random commits I just looked up:

I think the first example is is a clear example of a feat, as it adds explicit checking. To me, that is enough metadata covered by the spec to indicate the correct meaning to me. Is there something you think we could change/add which would make this kind of change meaningfully different from feat?

@stoically
Copy link
Author

If you have a concrete example that might help me understand.

I think the second commit I looked up covers this. The commit changes the behavior of an existing feature (it doesn't fix or add something).

I think the first example is is a clear example of a feat, as it adds explicit checking.

That's what happened internally, yes. It's not what happened in terms of "public features" - what happened for consumers with the first commit is that a feature (having support for Node 8) was dropped.

@stoically
Copy link
Author

stoically commented Sep 30, 2020

Remember this is about "semverness"

If we take semver's

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards compatible manner, and
PATCH version when you make backwards compatible bug fixes.

we can see that MAJOR and MINOR cover "add functionality" and "API changes". Both MAJOR and MINOR can be of type feat, and hence I'd like to see feat cover "API changes" as well.

And I guess "add functionality" instead of "add feature" helps to better grasp that functionality can also be added to existing features (and hence changing an existing feature, which, yes, technically is "add feature to feature").

@stovberpv
Copy link

Why can't just add another type cut (MAJOR)?

@tunnckoCore
Copy link
Member

tunnckoCore commented Oct 3, 2020

@wesleytodd Changing is either a fix to the existing feature or a new feat on top of the existing feature.

For me "change" mean any change. Not specifically a "new" or a fix. But anyway, I really agree with all of your comments.

@stoically

It clearly shadowy states it correlates to SemVer MINOR, from where we see that the most important and distinctive factor is... is it breaking or not, both for minor and major. So, feat can be anything "new; backward-compatible; including a fix, and so any changes to private / internal APIs".

That said, @stoically, your question should be "Is that 'drop' touch public APIs, does it break something?". If not, if it's dropping/changing private/internal APIs, then... no, it's not breaking, so it's can be labeled as feat or whatever.

what happened for consumers with the first commit is that a feature (having support for Node XXX) was dropped.

Technically, it's not a real/actual feature anymore at the time of "dropping" it, it's a burden - for both parties, maintainers and users. So in reality, removing it is "a feature" both in terms of SemVer and CC. 😆

In general, I 💯 agree that this text isn't good enough. Maybe "... when a commit changes a feature ...", or why not just include the "correlates to SemVer MINOR" in the spec section, because now it's somewhere above in the summary just as some recommendation or I don't know what.

@stoically
Copy link
Author

I wonder, did the meeting happen and if so, what was the outcome with regards to this issue?

@jolars
Copy link

jolars commented Jun 8, 2022

If feat is used for changes and/or removal of features, then won't that make it harder to generate change logs automatically?

I think it's wiser to use another keyword such as cut or remove. And I guess there's nothing stopping anyone right now from using cut along with BREAKING CHANGE or !, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants