[Feature] Adding renameSome method for selectively renaming multiple columns at once#91
[Feature] Adding renameSome method for selectively renaming multiple columns at once#91anusha5695 wants to merge 2 commits intoGmousse:developfrom
Conversation
|
Hi @anusha5695, I think I will create another PR in order to complete your work and apply what I said about the The aim is:
I hope you can review this. |
|
Hi @Gmousse: Okay. Can i pick that if that's not a problem? |
|
And this - "Just fix the few details (nothing about mechanics, just about notation)" . What should be fixed? I ll put them in the next commits. |
|
Hey ! Of course you can pick it. Go Go ! :)
Just the few comments I made about doc, or code style. Sorry, that was not clear :D. |
|
Cool. will push the changes over the weekend |
|
Hi @Gmousse : I guess the dist and bin folder would be built. Right? Also have pushed in the new changes. Please review :) |
|
Hi @Gmousse have you started reviewing the changes? |
Gmousse
left a comment
Gmousse
left a comment
There was a problem hiding this comment.
That seems nice.
But, just keep in mind that "deprecated" is not "deleted".
See my comments to have more informations.
|
|
||
| /** | ||
| * Rename selective columns. | ||
| * @param {Map} columnsMap Key value pairs of columns to rename and new column names of the DataFrame. |
There was a problem hiding this comment.
Consider replacing Map (in the description) by an Object.
Indeed, it could confuse the users with the real Map type https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map.
| * df.renameSome({'column1':'columnA', 'column3':'columnB', 'column50':'columnC']) | ||
| */ | ||
| renameSome(columnsMap) { | ||
| let availableColumnNames = this[__columns__]; |
There was a problem hiding this comment.
In order to avoid mutations in the current DataFrame by setting the array index, consider to use:
const availableColumnNames = Array.from(this[__columns__]);
| let availableColumnNames = this[__columns__]; | ||
| Object.entries(columnsMap).forEach(([key,value])=>{ | ||
| let position = availableColumnNames.indexOf(key); | ||
| position > -1 && (availableColumnNames[position] = value); |
There was a problem hiding this comment.
if (position > -1) {
availableColumnNames[position] = value
}
Your syntax is good, but it could confuse newcomers. Prefer to use a simple if statement in this case.
| .renameSome({}) | ||
| .listColumns(), | ||
| ["c2", "c3", "c4"], | ||
| "renamed selectively" |
There was a problem hiding this comment.
Replace the description by something like:
renamed nothing if the payload is empty.
| * @example | ||
| * df.renameAll(['column1', 'column3', 'column4']) | ||
| */ | ||
| renameAll(newColumnNames) { |
There was a problem hiding this comment.
The renameAll must not be deleted but deprecated.
It means that the method must remain and will be deleted on the next major version (2.0.0).
Moreover the method must throw a warning when used.
| * @example | ||
| * df.rename('column1', 'columnRenamed') | ||
| */ | ||
| rename(columnName, replacement) { |
There was a problem hiding this comment.
The current usage of rename must be kept.
Indeed we deprecate this usage (we don't delete it completely).
The renameAll must not be deleted but deprecated.
It means that the method must remain compatible with the current usage.The current usage will be completely deleted on the next major version (2.0.0).
Moreover the method must throw a warning when used with the current usage.
As a solution, you can check if columnsMap is an object or a string.
If it's a string you could kept the current usage.
Else use your new usage.
There was a problem hiding this comment.
(However you can update the docstring with your new usage)
There was a problem hiding this comment.
Hi @Gmousse Sorry I'm a lil confused.
As a solution, you can check if columnsMap is an object or a string.
If it's a string you could kept the current usage.
Else use your new usage
Didn't understand the above. Is it for rename or renameAll? cuz rename takes 2 params currently.
Sorry for the back and forth questions
There was a problem hiding this comment.
Hi !
Sorry to be late !
I was talking about .rename. Indeed we will handle parameters in order to ensure compatibility.
Example of implementation:
rename(mapping, deprecatedParam) {
if (typeof mapping === "string") {
// old behaviour
} else {
// your behaviour
}
}
| assert.deepEqual( | ||
| df | ||
| .select("c2", "c3", "c4") | ||
| .renameAll(["c16", "c17", "c18"]) |
There was a problem hiding this comment.
Keep this test until the real deprecation (on major version...)
|
Hi ! sorry to be late :D (I was busy and the vacation doesn't help) You can now read my review |
|
Hi @Gmousse : Sorry to bother on a vacation :D . Looked at the comments. Will make changes and push over the weekend |
3 participants
PR for #90