Skip to content
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

Rename get for categorical values? #142

Closed
bkamins opened this issue May 24, 2018 · 11 comments
Closed

Rename get for categorical values? #142

bkamins opened this issue May 24, 2018 · 11 comments

Comments

@bkamins
Copy link
Member

bkamins commented May 24, 2018

No description provided.

@bkamins bkamins closed this as completed May 24, 2018
@bkamins
Copy link
Member Author

bkamins commented May 24, 2018

I wanted to discuss if we should keep get as a function name for accessing value wrapped in categorical value but I convinced myself that it is OK so I closed the issue.

@nalimilan
Copy link
Member

nalimilan commented May 26, 2018

I'm not a fan of get. It was chosen because that function existed for Nullable, but now that it's been removed we could find something else (like unwrap).

@ararslan
Copy link
Member

Worth discussion, so reopening.

@ararslan ararslan reopened this May 26, 2018
@ararslan ararslan changed the title get for categorical values Rename get for categorical values? May 26, 2018
@bkamins
Copy link
Member Author

bkamins commented May 26, 2018

Unwrap is OK for me (I would wait till the discussion around coalesce in Base settles so that we know what names are used for what there as unwrap was also mentioned in the discussions). I understand that we would keep the current functionality (i.e. that unwrapping a missing should throw an error).

@bkamins
Copy link
Member Author

bkamins commented Oct 31, 2020

bump (if you prefer I can open a PR for this - but you probably have a better understanding of all the consequences of #310 to make this PR correctly)

@nalimilan
Copy link
Member

I don't think #310 would have any consequences on this.

The difficulty is that using something other than get will make CategoricalArrays even less generic, unless we define that function e.g. in DataAPI. But I'm not sure other packages need a similar concept. Any ideas?

@bkamins
Copy link
Member Author

bkamins commented Nov 1, 2020

I think it is the opposite: the concept of unwrapping is CategoricalArrays.jl specific.

CC the usual team: @pdeffebach @quinnj @oxinabox

@quinnj
Copy link
Member

quinnj commented Nov 2, 2020

We currently define unwrap in Tables.jl for unwrapping DataValues. I'd be happy to move that to DataAPI and we could use that in CategoricalArrays. I agree that get isn't a great method for the functionality here.

@bkamins
Copy link
Member Author

bkamins commented Nov 2, 2020

Ah - interesting. It would make sense. Though, looking at the implementation we would have to change the implementation a bit as now unwrap seems to be DataValues targetted only but at the same time there is no restriction on type of x.

@nalimilan
Copy link
Member

Ah, yes, that would make sense.

@nalimilan
Copy link
Member

Fixed by #328.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants