Skip to content

TextLoader should have only one constructor, that doesn't take Arguments as a parameter #1611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
markusweimer opened this issue Nov 13, 2018 · 4 comments
Assignees
Labels
API Issues pertaining the friendly API
Milestone

Comments

@markusweimer
Copy link
Member

markusweimer commented Nov 13, 2018

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.

@markusweimer markusweimer added the API Issues pertaining the friendly API label Nov 13, 2018
@markusweimer
Copy link
Member Author

/cc @Zruty0

@Zruty0
Copy link
Contributor

Zruty0 commented Nov 14, 2018

@markusweimer that's not exactly correct. TextLoader takes Arguments as a parameter, and the arguments class has a field called Column. 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 have
loader=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.Arguments needs to be public altogether. We should improve the other constructor to have non-advanced parameters, and also do #1515 .

@Zruty0 Zruty0 changed the title TextLoader should use columns as a parameter, not column TextLoader should have only one constructor, that doesn't take Arguments as a parameter Nov 14, 2018
@Zruty0
Copy link
Contributor

Zruty0 commented Nov 14, 2018

I took a liberty to update the issue name and description. I kept the old one for history

@eerhardt
Copy link
Member

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:

  1. With the simple/common options as direct parameters.
  2. If a component needed to have public "advanced" options, have another constructor/factory that took only the Options instance.

@shauheen shauheen added this to the 1218 milestone Dec 11, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API
Projects
None yet
Development

No branches or pull requests

5 participants