Improve Column Renaming for Selected Columns #9282
Conversation
|
@N-thony I can't see what has changed here. Can you add more details. |
|
@rdstern, do column selection first and try to rename column from the current column selection with any option from the Rename dialogue |
|
@rdstern have you re-tested this? |
There was a problem hiding this comment.
@N-thony I think your change may be working. But there are various problems, either from the changes, or perhaps always there?
a) whenever I click on rename I get the developer error.
b) When I did the select shown below I only got 2 columns visible out of the 3. I got the first column to be visible, by scrolling left, so no big deal. But whern I now do a new select, only the 2 columns appear. Of course I'd like them all, not just those in the current select, because I'm doing a new select. Maybe when new select is chosen there should be an unselect first?
@rdstern, this is solved now. |
@rdstern @lilyclements this needs your review. |
|
@N-thony not for this release, but could you resolve the conflicts? Then I'll check again. |
There was a problem hiding this comment.
- @rdstern: The VB.Net code looks good.
- @N-thony: I noticed several instances of
any(is.na(x)). In most cases, I believeanyNA()would suffice. However, please double-check to ensure it doesn't introduce the bug mentioned in [this comment](Paste no longer works in the test version #9207 (comment)) and issue Errors in length > 1 #9238. - @lilyclements: Could you kindly review the R code and merge the PR once you are satisfied?
Let me know if further refinements are needed!
|
|
||
| column_names <- self$get_column_names() | ||
|
|
||
| if (any(is.na(column_names))) { |
| @@ -1018,6 +1075,16 @@ DataSheet$set("public", "rename_column_in_data", function(curr_col_name = "", ne | |||
| for (i in seq_along(new_col_names)) { | |||
| self$append_to_variables_metadata(new_col_names[i], name_label, new_col_names[i]) | |||
| } | |||
|
|
|||
| column_names <- self$get_column_names() | |||
| if (any(is.na(column_names))) { | |||
|
|
||
| column_names <- self$get_column_names() | ||
|
|
||
| if (any(is.na(column_names))) { |
| if(missing(new_values)) stop("new_values is required") | ||
|
|
||
| # Find the column selection to update | ||
| if (is.null(column_selection_name)) { | ||
| stop("A column selection name must be provided to update the selection.") | ||
| } |
There was a problem hiding this comment.
Some inconsistency here. In the first instance you refer to the parameter name which is missing, in the second you refer to the objective of the missing parameter name.
Can you be consistent with this error message between them both (e.g., either say "Parameter for new values is missing", or "column_selection_name is required").
If this is code that the user wouldn't look at, then saying the parameter name makes sense. If this is code that the user (who may not be versed in R) will look at, then saying a statement like your second one makes more sense?
| @@ -921,6 +921,40 @@ DataSheet$set("public", "cor", function(x_col_names, y_col_name, use = "everythi | |||
| } | |||
| ) | |||
|
|
|||
| DataSheet$set("public", "update_selection", function(new_values, column_selection_name = NULL) { | |||
There was a problem hiding this comment.
Since you set it to be an error if column_selection_name = NULL, then you can just set column_selection_name here, and run it like you have run new_values (i.e., with if(missing(column_selection_name)) ... - unless I'm missing something?
N-thony
added
enhancement
Consistency
|
@Patowhiz can you check and merge? |
Fixes partially #9265
@rdstern this PR tempts to fix the issue when renaming selected columns. It completes issue #9265. Have a look