You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
You will notice something curious. This is an array of Column[], yet is named Column, not Columns. The reason for this is somewhat odd: this settings object originally arose out of the need to provide a command line settable object back when this code was supporting a tool (and not an API!). And, the presence of an array indicate that this setting can be set multiple times... so one could say, for instance, column=Foo column=Bar column=Biz, and so wind up with this field populated with 3 items. In this setting the name "column" is the most natural name.
Now we are trying to ship an API. What we did for the convenience of the command line now causes confusion for an API user, because they see an array (which can clearly handle multiple items), yet it has a singular name!
Fortunately, we can have our cake and eat it too: the ArgumentAttribute has something called Name, to allow the field name to be distinguished from the command line name. So, this:
and remain equivalent from the command line perspective, while not being confusing from the API perspective.
As a general rule any field with an ArgumentAttribute attribute on it that is also an array, we have often not made plural, for the aforementioned reasons. If it is singular:
Make the field name itself plural and
Retain the old singular command line as this Name parameter, so as to retain the benefit of why we named it this way in the first place.
However, please, not thoughtlessly! Think about whether that change makes sense in context.
Uh oh!
There was an error while loading. Please reload this page.
Consider this content of concatentating transformer's
Argument
's class:machinelearning/src/Microsoft.ML.Data/Transforms/ColumnConcatenatingTransformer.cs
Lines 116 to 117 in 496e185
Or this similar entry:
machinelearning/src/Microsoft.ML.Data/Transforms/ColumnConcatenatingTransformer.cs
Lines 122 to 123 in 496e185
You will notice something curious. This is an array of
Column[]
, yet is namedColumn
, notColumns
. The reason for this is somewhat odd: this settings object originally arose out of the need to provide a command line settable object back when this code was supporting a tool (and not an API!). And, the presence of an array indicate that this setting can be set multiple times... so one could say, for instance,column=Foo column=Bar column=Biz
, and so wind up with this field populated with 3 items. In this setting the name "column" is the most natural name.Now we are trying to ship an API. What we did for the convenience of the command line now causes confusion for an API user, because they see an array (which can clearly handle multiple items), yet it has a singular name!
Fortunately, we can have our cake and eat it too: the
ArgumentAttribute
has something calledName
, to allow the field name to be distinguished from the command line name. So, this:could become this:
and remain equivalent from the command line perspective, while not being confusing from the API perspective.
As a general rule any field with an
ArgumentAttribute
attribute on it that is also an array, we have often not made plural, for the aforementioned reasons. If it is singular:Make the field name itself plural and
Retain the old singular command line as this
Name
parameter, so as to retain the benefit of why we named it this way in the first place.However, please, not thoughtlessly! Think about whether that change makes sense in context.
/cc @stephentoub
The text was updated successfully, but these errors were encountered: