-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
Added type to DataFrame.rename mapper #347
Conversation
Thanks for your PR! |
@@ -2777,8 +2781,9 @@ export default class DataFrame extends NDframe implements DataFrameInterface { | |||
const colsAdded: string[] = []; | |||
const newColumns = this.columns.map(col => { | |||
if (mapper[col] !== undefined) { | |||
colsAdded.push(mapper[col]); | |||
return mapper[col] | |||
const newCol = `${mapper[col]}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Devwulf I'm not sure why you did this, can you explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Devwulf I'm not sure why you did this, can you explain?
So with the new mapper type, you can type in either a string
or a number
as the value in a mapping pair. Since the dataframe columns array only accept strings, line 2784 basically converts each mapped value as strings. This is so they can be pushed into the colsAdded
array and so that newColumns
stay as a string[]
instead of being a (string | number)[]
array.
Perhaps this function can also be separated into two with the only difference being the axis value they take in, but you might as well start doing this to all functions that take in an axis
option, which might complicate things. This is what I'm thinking about:
rename(
mapper: {
[index: string]: string
},
options?: {
axis?: 1, // this function is returned if axis is 1
inplace?: boolean
}
): DataFrame | void {
// Rename the columns
}
rename(
mapper: {
[index: string | number]: string | number
},
options?: {
axis?: 0, // this function is returned if axis is 0
inplace?: boolean
}
): DataFrame | void {
// Rename the row indexes
}
It might be convenient to do something similar as well for the inplace
option, so that the function returns the proper values (either void or DataFrame, not both) depending on the inplace
option, since I find myself always casting to DataFrame every time inplace
is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Devwulf I'm not sure why you did this, can you explain?
So with the new mapper type, you can type in either a
string
or anumber
as the value in a mapping pair. Since the dataframe columns array only accept strings, line 2784 basically converts each mapped value as strings. This is so they can be pushed into thecolsAdded
array and so thatnewColumns
stay as astring[]
instead of being a(string | number)[]
array.Perhaps this function can also be separated into two with the only difference being the axis value they take in, but you might as well start doing this to all functions that take in an
axis
option, which might complicate things. This is what I'm thinking about:rename( mapper: { [index: string]: string }, options?: { axis?: 1, // this function is returned if axis is 1 inplace?: boolean } ): DataFrame | void { // Rename the columns } rename( mapper: { [index: string | number]: string | number }, options?: { axis?: 0, // this function is returned if axis is 0 inplace?: boolean } ): DataFrame | void { // Rename the row indexes }It might be convenient to do something similar as well for the
inplace
option, so that the function returns the proper values (either void or DataFrame, not both) depending on theinplace
option, since I find myself always casting to DataFrame every timeinplace
is false.
Oh my bad, I see why now. Thanks for taking out time to explain, and yes you're correct, columns only take in string values.
@@ -2777,8 +2781,9 @@ export default class DataFrame extends NDframe implements DataFrameInterface { | |||
const colsAdded: string[] = []; | |||
const newColumns = this.columns.map(col => { | |||
if (mapper[col] !== undefined) { | |||
colsAdded.push(mapper[col]); | |||
return mapper[col] | |||
const newCol = `${mapper[col]}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Devwulf I'm not sure why you did this, can you explain?
So with the new mapper type, you can type in either a
string
or anumber
as the value in a mapping pair. Since the dataframe columns array only accept strings, line 2784 basically converts each mapped value as strings. This is so they can be pushed into thecolsAdded
array and so thatnewColumns
stay as astring[]
instead of being a(string | number)[]
array.Perhaps this function can also be separated into two with the only difference being the axis value they take in, but you might as well start doing this to all functions that take in an
axis
option, which might complicate things. This is what I'm thinking about:rename( mapper: { [index: string]: string }, options?: { axis?: 1, // this function is returned if axis is 1 inplace?: boolean } ): DataFrame | void { // Rename the columns } rename( mapper: { [index: string | number]: string | number }, options?: { axis?: 0, // this function is returned if axis is 0 inplace?: boolean } ): DataFrame | void { // Rename the row indexes }It might be convenient to do something similar as well for the
inplace
option, so that the function returns the proper values (either void or DataFrame, not both) depending on theinplace
option, since I find myself always casting to DataFrame every timeinplace
is false.
Oh my bad, I see why now. Thanks for taking out time to explain, and yes you're correct, columns only take in string values.
Fixes issue #345
Makes the DataFrame.rename
mapper
arg have a type of:instead of any. Even though the
options
arg can still be mistaken as the mapper arg, between the previousoptions
arg with the mapper option inside it and the current system where the mapper option is outside, the difference should be enough that Typescript would be able to warn about this new change when migrating to version 1.0.0.