Naming TODOs
Sebastien Pouliot
> Index: rules/Gendarme.Rules.Naming/DoNotUseReservedInEnumValueNamesRule.cs
> ===================================================================
> --- rules/Gendarme.Rules.Naming/DoNotUseReservedInEnumValueNamesRule.cs (revision 139717)
> +++ rules/Gendarme.Rules.Naming/DoNotUseReservedInEnumValueNamesRule.cs (working copy)
> @@ -33,14 +33,11 @@
>
> namespace Gendarme.Rules.Naming {
>
> - // TODO: How is renaming a reserved enum a breaking change, but adding a new
> - // enum is not?
> -
> /// <summary>
> /// This rule checks for enumerations that contain values named <c>reserved</c>. This
> /// practice, often seen in C/C++ applications, is not needed in .NET since adding new
> /// values is (normally) not a breaking change. However renaming a <c>reserved</c> value to a new
> - /// name would be a breaking change.
> + /// can be a breaking change because there is no way to prevent people from using it.
IIRC the MS breaking change definition is about "things that may break
compatibility", so renaming any enum member is something that "may
break", so a breaking change (for the exact reason you described). So we
should replace "can be a breaking change" with "is a breaking
change" (MS terminology) or "can break compatibility" (non-MS
terminology).
Otherwise this is ok to commit.
Thanks!
Sebastien
Jesse Jones
On Aug 12, 2009, at 7:16 AM, Sebastien Pouliot wrote:
>
>
>> Index: rules/Gendarme.Rules.Naming/
>> DoNotUseReservedInEnumValueNamesRule.cs
>> ===================================================================
>> --- rules/Gendarme.Rules.Naming/
>> DoNotUseReservedInEnumValueNamesRule.cs (revision 139717)
>> +++ rules/Gendarme.Rules.Naming/
>> DoNotUseReservedInEnumValueNamesRule.cs (working copy)
>> @@ -33,14 +33,11 @@
>>
>> namespace Gendarme.Rules.Naming {
>>
>> - // TODO: How is renaming a reserved enum a breaking change,
>> but adding a new
>> - // enum is not?
>> -
>> /// <summary>
>> /// This rule checks for enumerations that contain values
>> named <c>reserved</c>. This
>> /// practice, often seen in C/C++ applications, is not
>> needed in .NET since adding new
>> /// values is (normally) not a breaking change. However
>> renaming a <c>reserved</c> value to a new
>> - /// name would be a breaking change.
>> + /// can be a breaking change because there is no way to
>> prevent people from using it.
>
> IIRC the MS breaking change definition is about "things that may break
> compatibility", so renaming any enum member is something that "may
> break", so a breaking change (for the exact reason you described).
> So we
> should replace "can be a breaking change" with "is a breaking
> change" (MS terminology)
But afaik renaming an enum is only a breaking change if people are
actually using that enum. This should not ordinarily happen so I'd
prefer to use the weaker "can break" instead of will break.
> or "can break compatibility" (non-MS
> terminology).
This sounds right: I'd expect quite a few people will not know what
"breaking" means in this context so we'd probably be better off using
"breaking binary compatibility".
-- Jesse
Sebastien Pouliot
True, but that is always the case. E.g. if I remove (or rename) a
visible method (or type) then it's only "breaking" if someone use it.
Yet this is, in MS terminology, always a "breaking change" since the
implied forward compatibility promise is broken. This is a place where
the "FX" in FxCop shows - it was designed to check *shared libraries*
(not applications) and the rules/documentation/terminology reflect this.
Note that a "breaking change" is not necessarily bad (e.g.
applications) at least when the developer is aware of it. Of course a
framework generally has stricter rules for such changes.
> This should not ordinarily happen so I'd
> prefer to use the weaker "can break" instead of will break.
Ok, but let's do it in a way / terminology that minimize confusion with
MS "breaking change".
> > or "can break compatibility" (non-MS
> > terminology).
>
> This sounds right: I'd expect quite a few people will not know what
> "breaking" means in this context so we'd probably be better off using
> "breaking binary compatibility".
Yeah, this sounds like (needed) FAQ material - whatever we use to
document the rules.
Sebastien
Jesse Jones
Yeah, but what constitutes a breaking change is not easy to figure
out. I don't think I have ever seen a document describing what its
breaking and what is not breaking from the perspective of a C# coder
for example. And even at the CLI level it's hard to find this out.
Because of this I think it's useful to be clear about why the change
may be breaking, especially in this case where it won't in fact be
breaking unless users do something silly.
What if we used language like the below?
This rule checks for enumerations that contain values named
<c>reserved</c>. This practice, often seen in C/C++ applications, is
not needed in .NET since adding new values will not normally break
binary compatibility. However renaming a <c>reserved</c> enum value to
a new value can be a breaking change because there is no way to
prevent people from using the old value.
-- Jesse
Sebastien Pouliot
There was one back in 1.x days and MS provided the winchurn tool to
compare assemblies. I have not seen this being updated (well I did not
really look either) for 2.0 (e.g. generics).
> And even at the CLI level it's hard to find this out.
> Because of this I think it's useful to be clear about why the change
> may be breaking, especially in this case where it won't in fact be
> breaking unless users do something silly.
Yes. I like the extra details (the "why") and adding this reduce
potential confusion unless we reuse the same terminology as MS (fxcop)
with a similar, but not totally identical, meaning.
> What if we used language like the below?
>
> This rule checks for enumerations that contain values named
> <c>reserved</c>. This practice, often seen in C/C++ applications, is
> not needed in .NET since adding new values will not normally break
> binary compatibility. However renaming a <c>reserved</c> enum value to
> a new value can be a breaking change because there is no way to
> prevent people from using the old value.
The main problem, "can be a breaking change"*, is still in this
version. To minimize confusion we need to either:
a) follow MS terminology, i.e. "is a breaking change", because MS
(fxcop) design is based on the fact that a framework cannot know all its
API consumers, so it's bound to break somewhere.
I agree that *FX*cop definition is of limited value to most
people (outside MS) since most people write applications, not
frameworks. However this is how (past and present) fxcop people
understands a "breaking change".
b) provide our own description, i.e. "can break binary compatibility".
This has the advantage that we can promote a less FX-centric definition.
In both cases (i.e. for a or b) we need to add the definition(s), (MS
and ours if different, to the FAQ.
Sebastien
* "can be a breaking change" is used differently by fxcop, when a defect
solution may, or may not, result in a breaking change (e.g. multiple
solution choices). That is the "solution" itself, irrespective of the
consumer.
Jesse Jones
On Aug 14, 2009, at 8:36 AM, Sebastien Pouliot wrote:
>
> On Thu, 2009-08-13 at 09:55 -0700, Jesse Jones wrote:
>>
>> What if we used language like the below?
>>
>> This rule checks for enumerations that contain values named
>> <c>reserved</c>. This practice, often seen in C/C++ applications, is
>> not needed in .NET since adding new values will not normally break
>> binary compatibility. However renaming a <c>reserved</c> enum value
>> to
>> a new value can be a breaking change because there is no way to
>> prevent people from using the old value.
>
> The main problem, "can be a breaking change"*, is still in this
> version. To minimize confusion we need to either:
>
> a) follow MS terminology, i.e. "is a breaking change", because MS
> (fxcop) design is based on the fact that a framework cannot know all
> its
> API consumers, so it's bound to break somewhere.
>
> I agree that *FX*cop definition is of limited value to most
> people (outside MS) since most people write applications, not
> frameworks. However this is how (past and present) fxcop people
The problem is that "breaking" is a core concept in FxCop: every rule
includes a note about whether fixing the defect is likely to be a
breaking change. So FxCop users are (for the most part) going to know
what breaking means. I don't think this is true of Gendarme users,
unless they are also FxCop users which we should not assume.
>
> understands a "breaking change".
>
> b) provide our own description, i.e. "can break binary compatibility".
> This has the advantage that we can promote a less FX-centric
> definition.
I think this is the right answer (at least for now) and what I tried
to do with the new language above.
>
>
> In both cases (i.e. for a or b) we need to add the definition(s), (MS
> and ours if different, to the FAQ.
I think "breaking binary compatibility" is pretty straight forward.
Although it would be nice to have a FAQ covering some common cases of
breaking and non-breaking changes.
-- Jesse
Sebastien Pouliot
No need to assume, we got a survey about that :-) and about 31% of
Gendarme users are never using FxCop.
http://pages.infinit.net/ctech/gendarme-survey-2009.html
In fact it could be lower than 31% since some people reported "Visual
Studio built-in code analyzer" as "other" (while I meant that to be the
same as FxCop, my bad for not being explicit enough).
> >
> > understands a "breaking change".
> >
> > b) provide our own description, i.e. "can break binary compatibility".
> > This has the advantage that we can promote a less FX-centric
> > definition.
>
> I think this is the right answer (at least for now) and what I tried
> to do with the new language above.
Not quite. If the text use "breaking change" then it must have the same
meaning as FxCop, which in this case "is (not can be) a breaking
change".
> >
> >
> > In both cases (i.e. for a or b) we need to add the definition(s), (MS
> > and ours if different, to the FAQ.
>
> I think "breaking binary compatibility" is pretty straight forward.
> Although it would be nice to have a FAQ covering some common cases of
> breaking and non-breaking changes.
If the text is changed to use "breaking binary compatibility" then it
can be used with "can be". In that case both definitions will be added
to the FAQ.
Sebastien
Jesse Jones
Well, we shouldn't use "breaking change". We should use "breaking
binary compatibility".
>
>>>
>>>
>>> In both cases (i.e. for a or b) we need to add the definition(s),
>>> (MS
>>> and ours if different, to the FAQ.
>>
>> I think "breaking binary compatibility" is pretty straight forward.
>> Although it would be nice to have a FAQ covering some common cases of
>> breaking and non-breaking changes.
>
> If the text is changed to use "breaking binary compatibility" then it
> can be used with "can be". In that case both definitions will be added
> to the FAQ.
Well, do you think this language is acceptable?
This rule checks for enumerations that contain values named
<c>reserved</c>. This practice, often seen in C/C++ applications, is
not needed in .NET since adding new values will not normally break
binary compatibility. However renaming a <c>reserved</c> enum value to
a new value can be a breaking change because there is no way to
prevent people from using the old value.
-- Jesse
Sebastien Pouliot
egg-xactly my point :)
> >
> >>>
> >>>
> >>> In both cases (i.e. for a or b) we need to add the definition(s),
> >>> (MS
> >>> and ours if different, to the FAQ.
> >>
> >> I think "breaking binary compatibility" is pretty straight forward.
> >> Although it would be nice to have a FAQ covering some common cases of
> >> breaking and non-breaking changes.
> >
> > If the text is changed to use "breaking binary compatibility" then it
> > can be used with "can be". In that case both definitions will be added
> > to the FAQ.
>
> Well, do you think this language is acceptable?
>
> This rule checks for enumerations that contain values named
> <c>reserved</c>. This practice, often seen in C/C++ applications, is
> not needed in .NET since adding new values will not normally break
> binary compatibility. However renaming a <c>reserved</c> enum value to
> a new value can be a
***
> breaking change
***
> because there is no way to
> prevent people from using the old value.
Nope. From MS definition renaming a enum value *is* (not "can be") a
breaking change. What about:
This rule checks for enumerations that contain values named
<c>reserved</c>. This practice, often seen in C/C++ sources, is
not needed in .NET since adding new values will not normally
break binary compatibility, while renaming a <c>reserved</c>
enum value can since there is no way to prevent people from
using the old value.
where:
* "breaking change" is absent and everything applies to the same "break
binary compatibility";
* "C/C++ applications" is changed to "C/C++ sources" (even if that's
unrelated to the current discussion)
Sebastien
Jesse Jones
I think that language is better, but I am a bit befuddled by why you
think it is more correct.
>
>
> * "C/C++ applications" is changed to "C/C++ sources" (even if that's
> unrelated to the current discussion)
That sounds better too.
I've included a new patch. The only difference is that I am using your
text, but I fixed the run-on sentence and kept "however" instead of
"while".
-- Jesse
Sebastien Pouliot
Looks like the patch was badly encoded, but since you described the
changes in the email then it should be ok to commit (I'll read it from
svn).
Thanks
Sebastien
On Wed, 2009-08-26 at 09:23 -0700, Jesse Jones wrote:
> Index:=20DoNotUseReservedInEnumValueNamesRule.cs=0A=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A---=20=
> DoNotUseReservedInEnumValueNamesRule.cs=09(revision=20140725)=0A
> +++=20=
> DoNotUseReservedInEnumValueNamesRule.cs=09(working=20copy)=0A@@=20-33,14=20=
> +33,11=20@@=0A=20=0A=20namespace=20Gendarme.Rules.Naming=20{=0A=20=0A-=09=
> //=20TODO:=20How=20is=20renaming=20a=20reserved=20enum=20a=20breaking=20=
> change,=20but=20adding=20a=20new=0A-=09//=20enum=20is=20not?=0A-=0A=20=09=
> ///=20<summary>=0A=20=09///=20This=20rule=20checks=20for=20enumerations=20=
> that=20contain=20values=20named=20<c>reserved</c>.=20This=0A-=09///=20=
> practice,=20often=20seen=20in=20C/C++=20applications,=20is=20not=20=
> needed=20in=20.NET=20since=20adding=20new=0A-=09///=20values=20is=20=
> (normally)=20not=20a=20breaking=20change.=20However=20renaming=20a=20=
> <c>reserved</c>=20value=20to=20a=20new=0A-=09///=20name=20would=20be=20a=20=
> breaking=20change.=0A+=09///=20practice,=20often=20seen=20in=20C/C
> ++=20=
> sources,=20is=20not=20needed=20in=20.NET=20since=20adding=20new=0A
> +=09=
> ///=20values=20will=20not=20normally=20break=20binary=20compatibility.=20=
> However=20renaming=20a=20<c>reserved</c>=C2=A0=0A
> +=09///=20enum=20value=20=
> can=20since=20there=20is=20no=20way=20to=20prevent=20people=20from=20=
> using=20the=20old=20value.=0A=20=09///=20</summary>=0A=20=09///=20=
> <example>=0A=20=09///=20Bad=20example:=0AIndex:=20=
> AvoidDeepNamespaceHierarchyRule.cs=0A=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A---=20=
> AvoidDeepNamespaceHierarchyRule.cs=09(revision=20140725)=0A+++=20=
> AvoidDeepNamespaceHierarchyRule.cs=09(working=20copy)=0A@@=20-27,6=20=
> +27,7=20@@=0A=20//=0A=20=0A=20using=20System;=0A+using=20=
> System.ComponentModel;=0A=20=0A=20using=20Mono.Cecil;=0A=20=0A@@=20-36,8=20=
> +37,6=20@@=0A=20=0A=20namespace=20Gendarme.Rules.Naming=20{=0A=20=0A-=09=
> //=20TODO:=20Can=20users=20really=20change=20the=20default?=0A-=0A=20=09=
> ///=20<summary>=0A=20=09///=20This=20rule=20checks=20for=20deeply=20=
> nested=20namespaces=20within=20an=20assembly.=20It=20will=0A=20=09///=20=
> warn=20if=20the=20depth=20is=20greater=20than=20four=20(default=20value)=20=
> unless=20the=20fifth=20(or=20the=0A@@=20-89,8=20+88,13=20@@=0A=20=09=
> [EngineDependency=20(typeof=20(NamespaceEngine))]=0A=20=09public=20class=20=
> AvoidDeepNamespaceHierarchyRule=20:=20Rule,=20IAssemblyRule=20{=0A=20=0A=
> -=09=09private=20int=20max_depth=20=3D=204;=20//=20default=20value=0A
> +=09=
> =09private=20const=20int=20DefaultMaxDepth=20=3D=204;=0A
> +=09=09private=20=
> int=20max_depth=20=3D=20DefaultMaxDepth;=0A=20=0A+=09=09///=20=
> <summary>The=20depth=20at=20which=20namespaces=20may=20be=20nested=20=
> without=20triggering=20a=20defect.</summary>=0A+=09=09///=20=
> <remarks>Defaults=20to=204.</remarks>=0A+=09=09[DefaultValue=20=
> (DefaultMaxDepth)]=0A
> +=09=09[Description=20("The=20depth=20at=20which=20=
> namespaces=20may=20be=20nested=20without=20triggering=20a=20defect.")]=0A=
> =20=09=09public=20int=20MaxDepth=20{=0A=20=09=09=09get=20{=20return=20=
> max_depth;=20}=0A=20=09=09=09set=20{=0AIndex:=20=
> ParameterNamesShouldMatchOverridenMethodRule.cs=0A=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A---=20=
> ParameterNamesShouldMatchOverridenMethodRule.cs=09(revision=20140725)=0A=
> +++=20ParameterNamesShouldMatchOverridenMethodRule.cs=09(working=20copy)=0A=
> @@=20-35,13=20
> +35,12=20@@=0A=20=0A=20namespace=20Gendarme.Rules.Naming=20=
> {=0A=20=0A-=09//=20TODO:=20This=20really=20needs=20to=20explain=20why=20=
> this=20is=20a=20problem.=20Is=20it=20just=20the=20potential=20for=20a=0A=
> -=09//=20bit=20of=20confusion=20as=20the=20FxCop=20rule=20suggests=20or=20=
> are=20there=20more=20serious=20problems=20with=0A-=09//=20languages=20=
> like=20VB?=20=0A-=0A=20=09///=20<summary>=0A=20=09///=20This=20rule=20=
> warns=20if=20an=20overriden=20method's=20parameter=20names=20does=20not=20=
> match=20those=20of=20the=20=0A-=09///=20base=20class=20or=20those=20of=20=
> the=20implemented=20interface.=0A
> +=09///=20base=20class=20or=20those=20=
> of=20the=20implemented=20interface.=20This=20can=20be=20confusing=20=
> because=20it=20may=0A
> +=09///=20not=20always=20be=20clear=20that=20it=20=
> is=20an=20override=20or=20implementation=20of=20an=20interface=20method.=20=
> It=0A
> +=09///=20also=20makes=20it=20more=20difficult=20to=20use=20the=20=
> method=20with=20languages=20that=20support=20named=0A+=09///=20=
> parameters=20(like=20C#=204.0).=0A=20=09///=20</summary>=0A=20=09///=20=
> <example>=0A=20=09///=20Bad=20example:=0A=
>
Jesse Jones
On Aug 29, 2009, at 9:49 AM, Sebastien Pouliot wrote:
>
> Hello Jesse,
>
> Looks like the patch was badly encoded, but since you described the
> changes in the email then it should be ok to commit (I'll read it from
> svn).
r140938
-- Jesse