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

Added type to DataFrame.rename mapper #347

Merged
merged 1 commit into from
Jan 14, 2022
Merged

Conversation

Devwulf
Copy link
Contributor

@Devwulf Devwulf commented Jan 13, 2022

Fixes issue #345

Makes the DataFrame.rename mapper arg have a type of:

{
    [index: string | number]: string | number
}

instead of any. Even though the options arg can still be mistaken as the mapper arg, between the previous options 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.

@risenW
Copy link
Member

risenW commented Jan 14, 2022

Fixes issue #345

Makes the DataFrame.rename mapper arg have a type of:

{
    [index: string | number]: string | number
}

instead of any. Even though the options arg can still be mistaken as the mapper arg, between the previous options 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.

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]}`;
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

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.

@risenW risenW self-requested a review January 14, 2022 16:58
@@ -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]}`;
Copy link
Member

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.

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.

@risenW risenW merged commit a6e2cf4 into javascriptdata:dev Jan 14, 2022
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

Successfully merging this pull request may close these issues.

2 participants