Rename CreateTextReader to CreateTextLoader #2125
Conversation
najeeb-kazmi
requested review from
CESARDELATORRE,
wschin,
eerhardt,
TomFinley,
artidoro and
sfilipi
src/Microsoft.ML.Data/DataLoadSave/Text/TextLoaderSaverCatalog.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.ML.Data/DataLoadSave/Text/TextLoaderSaverCatalog.cs
Outdated
Show resolved
Hide resolved
| @@ -11,33 +11,33 @@ namespace Microsoft.ML | |||
| public static class TextLoaderSaverCatalog | |||
| { | |||
| /// <summary> | |||
| /// Create a text reader <see cref="TextLoader"/>. | |||
| /// Create a text loader <see cref="TextLoader"/>. | |||
| /// </summary> | |||
| /// <param name="catalog">The <see cref="DataOperations"/> catalog.</param> | |||
| /// <param name="columns">The columns of the schema.</param> | |||
| /// <param name="hasHeader">Whether the file has a header.</param> | |||
| /// <param name="separatorChar">The character used as separator between data points in a row. By default the tab character is used as separator.</param> | |||
| /// <param name="dataSample">The optional location of a data sample.</param> | |||
There was a problem hiding this comment.
/// The optional location of a data sample.</para [](start = 8, length = 74)
?Do you want to also create a small sample, for the apis of this file, and link it from here?
| /// </summary> | ||
| /// <param name="catalog">The <see cref="DataOperations"/> catalog.</param> | ||
| /// <param name="columns">The columns of the schema.</param> | ||
| /// <param name="hasHeader">Whether the file has a header.</param> | ||
| /// <param name="separatorChar">The character used as separator between data points in a row. By default the tab character is used as separator.</param> | ||
| /// <param name="dataSample">The optional location of a data sample.</param> | ||
| public static TextLoader CreateTextReader(this DataOperations catalog, | ||
| public static TextLoader CreateTextLoader(this DataOperations catalog, |
There was a problem hiding this comment.
CreateTextLoader [](start = 33, length = 16)
My personal take on this is: TextReader is a much better name than TextLoader. I would rather rename the TextLoader class to TextReader..
There was a problem hiding this comment.
This is correct, we are replacing the IDataLoader idiom with the IDataReader idiom, as part of #581 and much subsequent work.
There was a problem hiding this comment.
Ah. Good point,. Maybe we should actually yname IDataReader to IDataLoader? And call the existing legacy IDataLoader interface something like IDataLoaderLegacy? What we all think?
There was a problem hiding this comment.
Similar renames to IDataReaderEstimator to I suppose IDataLoaderEstimator.
There was a problem hiding this comment.
My point is to be consistent between method's name and returned type, so either on of the following would be good for me: 👍
public static TextLoader CreateTextLoader()
public static TextReader CreateTextReader()
There was a problem hiding this comment.
Sounds good. Can't name it reader, guess we have to reuse the old "loader" thing.
ghost
locked as resolved and limited conversation to collaborators
Fixes #1690
Rename
CreateTextReadertoCreateTextLoaderto conform to the return type: