search

andreasohlund / APIComparer

Compares NuGetPackages/Assemblies and displays changes in the public api.
MIT License
51 stars 4 forks source link

New data model for the exe #55

Closed andreasohlund closed 8 years ago

andreasohlund commented 8 years ago

There was alot of repetion in the old data model. Spiking a new model that focus more on "api changes" instead of type changes.

andreasohlund commented 8 years ago

The current new format looks like this (obsoletes on individual fields and methods still missing)

------------------- new format ------------------

Breaking changes

The following public types is no longer available

The following types have breaking changes

Example.ClassToBeObsoletedWithErrorInNextVersion

Obsoleted with Error - This type should no longer be used, use XYZ instead.

Example.ClassWithMembersToBeObsoletedWithErrorInNextVersion

Example.IMethodChangesParametersNextVersion

Removed methods

Example.MemberInternalNextVersion

Removed fields

Removed methods

Example.MemberMissingNextVersion

Removed fields

Removed methods

Non breaking changes

Example.ClassToBeObsoletedWithWarnInNextVersion

Obsoleted with Warning - This type should no longer be used, use XYZ instead. ------------------- end new format ------------------

@ParticularLabs/apicomparer what do you all think about the separation of breaking / non breaking changes?

SeanFeldman commented 8 years ago

+1 @andreasohlund

andreasohlund commented 8 years ago

Give that we have a non breaking section would it make any sense to show "new types" ? cc @SimonCropp

SimonCropp commented 8 years ago

Give that we have a non breaking section would it make any sense to show "new types"

Dont think so

danielmarbach commented 8 years ago

I think that would provide value.

Am 26.12.2015 um 09:38 schrieb Simon Cropp notifications@github.com:

Give that we have a non breaking section would it make any sense to show "new types"

Dont think so

— Reply to this email directly or view it on GitHub.

andreasohlund commented 8 years ago

Had a chat with @SimonCropp and we came to the conclusion that "obsolete with warn" a special form of removal sinces it "notifies of a upcoming apichange". While it's technically a breaking change for users that have treat warnings as errors on we still decided to treat them as non breaking changes since

  1. The implementions should still work
  2. The warning can be suppressed with a compiler directive

I created this mindmap as a result:

https://drive.google.com/a/nservicebus.com/file/d/0Bxjcg5ArjglPSDBSdGJrNlBQSzg/view?usp=sharing

Snapshot:

78f44e508df8013303e83d1857c415ff 1

Thoughts/Ideas/Comments?

danielmarbach commented 8 years ago

Don't we (particular) implement them with throw new NotImplementedException()? Which would be opposed to your brainstorming conclusion?

Am 26.12.2015 um 13:18 schrieb Andreas Öhlund notifications@github.com:

Had a chat with @SimonCropp and we came to the conclusion that "obsolete with warn" a special form of removal sinces it "notifies of a upcoming apichange". While it's technically a breaking change for users that have treat warnings as errors on we still decided to treat them as non breaking changes since

The implementions should still work The warning can be suppressed with a compiler directive I created this mindmap as a result:

https://drive.google.com/a/nservicebus.com/file/d/0Bxjcg5ArjglPSDBSdGJrNlBQSzg/view?usp=sharing

Snapshot:

Thoughts/Ideas/Comments?

— Reply to this email directly or view it on GitHub.

andreasohlund commented 8 years ago

Don't we (particular) implement them with throw new NotImplementedException()? Which would be opposed to your brainstorming conclusion?

For "obsolete with error" we do throw but not for "obsolete with warn". Does that make sense?

danielmarbach commented 8 years ago

I might be too tired but as far as I'm involved we mostly used obsolete with error, see

https://github.com/Particular/NServiceBus/blob/develop/src/NServiceBus.Core/obsoletes.cs

Even for APIs that have been introduced just for one version. Look at the obsoletes. Most of the messages are like: hey we don't compile anymore for V6 and will remove in v7, here is what you should be using.

And the impls throw. So where do we actually use obsolete with warning?

Am 26.12.2015 um 23:06 schrieb Andreas Öhlund notifications@github.com:

Don't we (particular) implement them with throw new NotImplementedException()? Which would be opposed to your brainstorming conclusion?

For "obsolete with error" we do throw but not for "obsolete with warn". Does that make sense?

— Reply to this email directly or view it on GitHub.

SimonCropp commented 8 years ago

@danielmarbach that is because we r in the phase of a major release.

danielmarbach commented 8 years ago

Let me rephrase:

How many times have we used obsolete as warn in the past?

Am 27.12.2015 um 00:45 schrieb Simon Cropp notifications@github.com:

@danielmarbach that is because we r in the phase of a major release.

— Reply to this email directly or view it on GitHub.

andreasohlund commented 8 years ago

5.X has a few

http://apicomparer-preview.particular.net/compare/nservicebus/5.0.0...5.1.3

On Sun, Dec 27, 2015 at 8:40 AM, Daniel Marbach notifications@github.com wrote:

Let me rephrase:

How many times have we used obsolete as warn in the past?

Am 27.12.2015 um 00:45 schrieb Simon Cropp notifications@github.com:

@danielmarbach that is because we r in the phase of a major release.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/ParticularLabs/APIComparer/pull/55#issuecomment-167391165 .

andreasohlund commented 8 years ago

I've got another idea, if we treat "obsolete with warn" as a notification of a upcoming breaking change we get the following layout (note that I added optional parsing of the obsolete string based on knowlege of ObsoleteEx)

------------ start -----------

The following types are no longer available

Example.MissingNextVersionClass

No upgrade instructions provided.

Example.InternalNextVersionClass

No upgrade instructions provided.

Example.ClassToBeObsoletedWithErrorInNextVersion

This type should no longer be used, use XYZ instead.

Types with removed members

Example.ClassWithMembersToBeObsoletedWithErrorInNextVersion

Removed fields

Removed methods

Example.IMethodChangesParametersNextVersion

Removed methods

Example.MemberInternalNextVersion

Removed fields

Removed methods

Example.MemberMissingNextVersion

Removed fields

Removed methods

The following will be removed in upcoming versions

Version - 2.0.0

Example.ClassToBeObsoletedWithWarnInNextVersion

This type should no longer be used, use XYZ instead. Will be treated as an error from version 2.0.0.

------------ end -----------

danielmarbach commented 8 years ago

Really cool!

Am 28.12.2015 um 11:17 schrieb Andreas Öhlund notifications@github.com:

I've got another idea, if we treat "obsolete with warn" as a notification of a upcoming breaking change we get the following layout (note that I added optional parsing of the obsolete string based on knowlege of ObsoleteEx)

------------ start -----------

The following types are no longer available

Example.MissingNextVersionClass

No upgrade instructions provided.

Example.InternalNextVersionClass

No upgrade instructions provided.

Example.ClassToBeObsoletedWithErrorInNextVersion

This type should no longer be used, use XYZ instead.

Types with removed members

Example.ClassWithMembersToBeObsoletedWithErrorInNextVersion

Removed fields

string StringField - Use xyz instead. Removed methods

string get_StringProperty() - Can safely be removed. void Method() - This is how to do it. void set_StringProperty(string) - Can safely be removed. Example.IMethodChangesParametersNextVersion

Removed methods

void MethodName(string) - No upgrade instructions provided. Example.MemberInternalNextVersion

Removed fields

string StringField - No upgrade instructions provided. Removed methods

string get_StringProperty() - No upgrade instructions provided. void Method() - No upgrade instructions provided. void set_StringProperty(string) - No upgrade instructions provided. Example.MemberMissingNextVersion

Removed fields

string StringField - No upgrade instructions provided. Removed methods

string get_StringProperty() - No upgrade instructions provided. void Method() - No upgrade instructions provided. void set_StringProperty(string) - No upgrade instructions provided. The following will be removed in upcoming versions

Version - 2.0.0

Example.ClassToBeObsoletedWithWarnInNextVersion

This type should no longer be used, use XYZ instead. Will be treated as an error from version 2.0.0.

------------ end -----------

— Reply to this email directly or view it on GitHub.

andreasohlund commented 8 years ago

Below is the final version of the new layout where also individual member obsoletions is shown with their correct version.

---------------- start new layout -------------

Changes in current version

The following types are no longer available

Example.ClassObsoletedWithWarnShouldBeIncludedIfRemoved

Some message.

Example.MissingNextVersionClass

No upgrade instructions provided.

Example.ClassObsoletedWithWarnShouldBeIncludedIfInternalized

Some message.

Example.InternalNextVersionClass

No upgrade instructions provided.

Example.ClassToBeObsoletedWithErrorInNextVersion

This type should no longer be used, use XYZ instead.

Types with removed members

Example.MemberMissingNextVersion

Removed fields

Removed methods

Example.IMethodChangesParametersNextVersion

Removed methods

Example.ClassWithMembersToBeObsoletedWithErrorInNextVersion

Removed fields

Removed methods

Example.MemberInternalNextVersion

Removed fields

Removed methods

Upcoming changes in Version - 2.0.0

The following types are no longer available

Example.ClassToBeObsoletedWithWarnInNextVersion

This type should no longer be used, use XYZ instead. Will be treated as an error from version 2.0.0.

Types with removed members

Example.ClassWithMembersToBeObsoletedWithWarnInNextVersion

Removed fields

Removed methods

---------------- end new layout -------------

andreasohlund commented 8 years ago

Since this only affects the exe I'm going to merge this. I'll do a different pull for the site once I've tested it IRL for a few weeks