Internalize ITransformTemplate #2334
Conversation
Ivanidzo4ka
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2334 +/- ##
==========================================
- Coverage 71.16% 71.16% -0.01%
==========================================
Files 780 780
Lines 140340 140338 -2
Branches 16048 16043 -5
==========================================
- Hits 99879 99869 -10
- Misses 36011 36019 +8
Partials 4450 4450
|
| @@ -97,7 +97,8 @@ public interface IDataTransform : IDataView, ICanSaveModel | |||
| /// Data transforms need to be able to apply themselves to a different input IDataView. | |||
| /// This interface allows them to implement custom rebinding logic. | |||
There was a problem hiding this comment.
logic [](start = 65, length = 5)
Until we can delete this altogether, it may be worthwhile to add a note here to explain that the preferred idioms is that things work via the new ITransformer mechanism, which is the successor of this sort of idiom. I might not suggest adding the [Obsolete] tag -- that's a bit heaviweight -- just something in the XML docstring here like, "yeah, yeah, this is here, but we're trying to get rid of it, don't write new code with it."
| @@ -682,7 +682,7 @@ public Row GetRow(Row input, Func<int, bool> active) | |||
| return new RowImpl(input, _mapper); | |||
| } | |||
|
|
|||
| public IDataTransform ApplyToData(IHostEnvironment env, IDataView newSource) | |||
| IDataTransform ITransformTemplate.ApplyToData(IHostEnvironment env, IDataView newSource) | |||
There was a problem hiding this comment.
ITransformTemplate [](start = 27, length = 18)
I agree with you that this is probably the best policy since it even discourages internal code from using this now deprecated idiom, but just in case you are curious, it is not necessary to do this here since this nested class (SelectColumnsDataTransform) is itself not public.
There was a problem hiding this comment.
Thanks @Ivanidzo4ka , just one very minor comment about documentation to discourage any future use, but feel free to ignore unless you find you have to make a different change for some other reason.
| IDataTransform ITransformTemplate.ApplyToData(IHostEnvironment env, IDataView newSource) | ||
| => ApplyToDataCore(env, newSource); | ||
| internal IDataView ApplyToData(IHostEnvironment env, IDataView newSource) | ||
| => ApplyToDataCore(env, newSource); |
There was a problem hiding this comment.
Why do you need this second method? IDataTransform derives from IDataView.
ghost
locked as resolved and limited conversation to collaborators
Towards #1995