Repository navigation
TextLoader should have only one constructor, that doesn't take Arguments as a parameter #1611
Description
TextLoader needs to hide the constructor that has Arguments, and also expose the HasHeader and SeparatorChars to be non-advanced parameters.
-- old description --
TextLoader presently accepts a parameter called column for the array of columns to read. That seems odd, given that almost always, multiple columns are passed. Hence, it would be more natural to call that parameter columns.
Activity
markusweimer commentedon Nov 13, 2018
/cc @Zruty0
Zruty0 commentedon Nov 14, 2018
@markusweimer that's not exactly correct.
TextLoadertakesArgumentsas a parameter, and the arguments class has a field calledColumn. This is actually a common pattern in our codebase to use singular nouns for things that are internally arrays: it's better in command line to haveloader=Text{column=Foo:TX:0 column=Bar:R4:1}than it is to have
loader=Text{columns=Foo:TX:0 columns=Bar:R4:1}.However, I don't think the constructor that takes
TextLoader.Argumentsneeds to be public altogether. We should improve the other constructor to have non-advanced parameters, and also do #1515 .[-]TextLoader should use `columns` as a parameter, not `column`[/-][+]TextLoader should have only one constructor, that doesn't take Arguments as a parameter[/+]Zruty0 commentedon Nov 14, 2018
I took a liberty to update the issue name and description. I kept the old one for history
eerhardt commentedon Nov 30, 2018
See dotnet/apireviews#81 for some notes from the API design discussion on this API. In the end, we decided to have 2 constructor/factories: