Skip to content

DataKind Enum is not well documented #2006

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
sfilipi opened this issue Jan 3, 2019 · 7 comments
Closed

DataKind Enum is not well documented #2006

sfilipi opened this issue Jan 3, 2019 · 7 comments
Labels
API Issues pertaining the friendly API

Comments

@sfilipi
Copy link
Member

sfilipi commented Jan 3, 2019

This issue was initially logged in the dotnet/docs repo 9278 and 9184.

@TomFinley, @CESARDELATORRE, @eerhardt Is the DataKind Enum something that will remain user facing, or will it get behind the .net types?

Should we document the DataKind or cleanup our error messages, and usage to not contain references to the DataKind anymore like #1943 suggest?

cc @mairaw @JRAlexander

@sfilipi sfilipi added the API Issues pertaining the friendly API label Jan 3, 2019
@TomFinley
Copy link
Contributor

TomFinley commented Jan 3, 2019

We do not want DataKind visible anywhere. This is why ColumnType now has it as private. It ought to be deleted. Anywhere currently using DataKind should use some other mechanism. The only place we might want something like it is a place like TextLoader or whatever, to describe what type of item should be loaded from whatever fields of the text file we are reading, but even then it is questionable. It should not be a first class citizen in dataview land.

I believe I've mentioned in this one of my prior issues, but possibly not. At any rate I'm having trouble finding it again. 😛

@JRAlexander
Copy link

JRAlexander commented Jan 3, 2019

@TomFinley - What should be used in place of DataKind for TextLoader field definition? Is there a best practice that you would recommend as you've said that use case is questionable? Thanks!

@TomFinley
Copy link
Contributor

@JRAlexander. In an ideal world. DataKind would be annihilated or at most internal, and TextLoader can use whatever it likes. If we chose to continue using an enum, that enum would be particular to the text loader class.

However what I'd prefer if we had a mechanism that didn't rely on enums at all: in particular, when I was writing the static API, when loading a text field, I didn't rely on enums but instead on overloads to say, "load float," "load bool."

public Scalar<float> LoadFloat(int ordinal) => Load<float>(DataKind.R4, ordinal);

However, I don't insist on that. This may just be personal preference, but while I think enums are great things and super useful, every time I see them used them in a way that intersects with polymorphism of types, it absolutely makes my skin crawl, since in a language like C# there are just so many more and better ways to handle type-polymorphism.

But, that may be just me. Other people seem to tolerate the enum just fine. So if we want to keep using an enum in TextLoader, that's, again, fine, I'll get over it. But I'd think it should be something very closely associated with TextLoader, properly documented, and certainly not something that seems to be associated with IDataView as some sort of priviledged "thing" (as DataKind is now in its privileged position).

@sfilipi
Copy link
Member Author

sfilipi commented Jan 3, 2019

Since I opened this, to make this issue actionable, and not just a discussion placeholder:

1- I'll rename this issue to: Substitute internal usage of DataKind by the actual .net respective System.Type. IMO that can be done post Project 13 but please review its pri.

2- There is already an issue to cleanup the error messages from mentioning DataKind.
This is already in Project 13.

3- Open an issue about investigating the TextLoader to potentially using the respective System.Type to load. Address backwards compatibility.

cc @TomFinley @shauheen @glebuk @CESARDELATORRE to arrange priorities.

@TomFinley
Copy link
Contributor

3- Open an issue about investigating the TextLoader to potentially using the respective System.Type to load. Address backwards compatibility.

See, that I might like even less than an enum, because it's less restrictive in a bad way. The set of types that the text loader can load is certainly finite. So, it should be somehow enumerated in a finite set, either in a finite set of an enum, or a finite set of calls with specific names (with the suggestion they are associated with specific types -- that type not being one provided by the user). Just clarifying, that while I don't like an enum, there are some things I might like even less than that. :)

@TomFinley
Copy link
Contributor

I'll try to explain what I mean perhaps better by analogy. If I consider the .NET BinaryReader class, I'd consider things in this order of desirability.

  1. Currently the class has methods like ReadChar, ReadDouble, ReadSingle, ReadInt32, and so forth. This to me seems very clear, and is happily how the .NET team wrote this class. This is more like what I wrote in static API, and somewhat what I'd like TextLoader to look like.

  2. Worse than this would be to have a single method of signature object Read(DataKind), which takes a DataKind enumerating all the different types BinaryReader might be capable of reading, and returning a boxed object. This describes the behavior of our TextLoader today, and I view that as bad. (Not disastrous, just obnoxious.)

  3. Far worse than either would be object Read(System.Type), which like the current implementation of BinaryReader operates over a finite set of types, but doesn't tell you which. This is what seemed to be suggested above, and I object strongly to that. I'd view that as disastrous.

@shauheen
Copy link
Contributor

As changes are happening to data types this is not going to be an issue. Please reopen if you have other concerns.

@TomFinley TomFinley mentioned this issue Feb 20, 2019
3 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 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

4 participants