Add convenience constructors for TextLoader. #698
Conversation
Creating a TextLoader and specifying its arguments manually is too verbose. We should add a few constructor overloads to make it easier.
eerhardt
requested review from
codemzs,
Ivanidzo4ka,
TomFinley,
zeahmed and
Zruty0
| : this(name, type, new[] { new Range(index) }) { } | ||
|
|
||
| public Column(string name, DataKind? type = null, Range[] source = null, KeyRange keyRange = null) | ||
| { |
There was a problem hiding this comment.
{ [](start = 12, length = 1)
Contracts.CheckValue(name, nameof(name)); Contracts.CheckValueOrNull(everything_else);
Also, what's up with range being nullable?
#Closed
There was a problem hiding this comment.
Which range? source or keyRange?
Both of them can be null today.
In reply to: 211433252 [](ancestors = 211433252)
There was a problem hiding this comment.
I meant source. What does source=null signify?
In reply to: 211438122 [](ancestors = 211438122,211433252)
There was a problem hiding this comment.
It was supposed to mean, "I haven't set it yet". Just like if you say:
new TextLoader.Column() { Name = "foo" }
today.
At first, I meant these constructors to be as non-prescriptive as the current parameterless constructors are. And the validation only happens once you actually use the object. But since we are adding Contracts checks into them, that reasoning probably doesn't hold anymore.
I'll remove the default null value for source, and will also need to remove it for type.
There was a problem hiding this comment.
Oh, I see. Well, I like it better this way. We didn't have 'dual-initialized' objects before in our codebase (either the ctor was completely empty, or it would always construct a valid immutable object), so there's no standard practice to follow in this case.
For the self-documentation purpose, also add Contracts.CheckValueOrNull(type)
In reply to: 211655552 [](ancestors = 211655552)
There was a problem hiding this comment.
For the self-documentation purpose, also add Contracts.CheckValueOrNull(type)
That isn't possible since CheckValueOrNull has a class generic constraint, and type is a Nullable<DataKind>
| @@ -179,6 +192,14 @@ public bool IsValid() | |||
|
|
|||
| public sealed class Range | |||
| { | |||
| public Range() { } | |||
|
|
|||
| public Range(int index) | |||
There was a problem hiding this comment.
Range [](start = 19, length = 5)
Does it make sense to add a ctor that takes min and max? #Closed
| public Range() { } | ||
|
|
||
| public Range(int index) | ||
| { |
There was a problem hiding this comment.
{ [](start = 11, length = 2)
Contracts.CheckParam(index >=0 ... #Closed
|
test OSX10.13 Debug |
ghost
locked as resolved and limited conversation to collaborators
Creating a TextLoader and specifying its arguments manually is too verbose. We should add a few constructor overloads to make it easier.
This work item is related to the new API proposal #371