-
Notifications
You must be signed in to change notification settings - Fork 103
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 more options to rename multiple columns #7208
Added more options to rename multiple columns #7208
Conversation
implementing a delete option on the cells of reogrid
@N-thony thanks for the discussion. Leave this to me for now. I will alert you when the R code is ready. |
@shadrackkibet sounds good. Thanks. |
instat/dlgName.vb
Outdated
If Not ucrReceiverColumns.IsEmpty() Then | ||
ucrBase.OKEnabled(True) | ||
Else | ||
ucrBase.OKEnabled(False) | ||
End If |
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.
The default is to rename all columns. Therefore if the receiver is empty we use the whole data frame. This means we do not need to disable OK if the multiple receiver is empty.
@N-thony I have now added the R code that will allow the renaming of multiple columns one by one via a grid. I have also added an option to edit variable labels.
You need to construct these data frames from the grid and pass them into the R function. Below is an illustration of how this should look like from the dialog;
|
@shadrackkibet thanks, I will go ahead and link it with the grid. |
@rdstern I have now linked the R code provided by Shadrack with the grid. I did some tests and seems fine. Could you test too? Thanks. |
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.
@N-thony so far I have just looked at the multiple. Not yet the rename_with.
a) Looks fine on manual changes.
b) When I paste from Excel they appear to change, but it doesn't register.
c) When I press Ok and have made a change, but not clicked elsewhere, then the last change doesn't register.
d) When I click to include labels it goes blank and freezes. (And, once it is working please test copying the old names and pasting into the variable labels.
@rdstern thanks for your comment. During our discussion we agreed to get the multiple option working well before moving to the rename_with option. Thanks for reminding about the pasting feature on the grid. I made some change regarding your comments above. I didn't yet finish to make a proper test to check if it is stable, but you may wish to test it too at this stage. |
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.
@N-thony that's progress!
a) Paste is working, but only for the first cell thaqt is pasted.
b) Adding labels is now sensible. I only checked pasting, and that seems the same as the names, so it just pastes the first label.
@rdstern I'm still looking into this. Meanwhile, @shadrackkibet I found that with the functions parameters passing the |
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.
@N-thony looks fine to me. I am approving, so it can be merged. The next item is the abbreviate function, which I hope will be easy to add?
@N-thony
If a control (Nud/input) is needed for
|
@shadrackkibet great to have this and a really good "quick win" for R-Instat. Once that is added and merged, then I suggest that the last feature in renaming (which overlaps with all your select stuff, i.e. your select is the find, and then we add replace!) becomes far less urgent, and could wait till later in the year. Certainly until you have everything you need in the Select stuff. So, I can write that as a new issue. (Maybe we don't even need the find, but it just works on a defined select! |
This should be straightforward to implement. In a nutshell, this will simply have a receiver to take in a select object then we just rename those columns in the select object. |
…unction and set it the controls to use those parameters
Exactly! |
@rdstern @shadrackkibet I have added this. |
instat/dlgName.vb
Outdated
dctRowsNewNameChanged.Clear() | ||
dctRowsNewLabelChanged.Clear() | ||
|
||
ucrNudAbbreviate.SetText(4) |
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.
If you have the control passing the parameter then the control will read from the R code.
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.
This is resolved
Private clsDefaultRFunction As New RFunction | ||
Private clsNewColNameDataframeFunction As New RFunction | ||
Private clsNewLabelDataframeFunction As New RFunction | ||
Private clsDummyFunction As New RFunction |
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.
Try using conditions. Then you wouldn't need a dummy function. Happy to discuss.
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.
clsDummyFunction concerns ucrChkIncludeLabels, and it will be improved in the implementation of the ucrControl appropriate for this grid
instat/dlgName.vb
Outdated
Private Function GetValuesAsVector(dctValues As Dictionary(Of Integer, String)) As String | ||
Dim strValue As String = "" | ||
Dim i As Integer | ||
strValue = strValue & "c(" | ||
For Each iRow As Integer In dctValues.Keys | ||
If i > 0 Then | ||
strValue = strValue & "," | ||
End If | ||
strValue = strValue & Chr(34) & dctValues(iRow) & Chr(34) | ||
i = i + 1 | ||
Next | ||
strValue = strValue & ")" | ||
Return strValue | ||
End Function | ||
|
||
Private Sub ValidateNamesFromDictionary(iColIndex As Integer) | ||
If iColIndex = 1 Then | ||
For Each value In dctRowsNewNameChanged.Values | ||
If Not CheckNames(value, iColIndex) Then | ||
MsgBox("The column name must not be a numeric or contains space or french accent or be a boolean e.g TRUE, FALSE, T, F.") | ||
bCurrentCell = False | ||
Exit For |
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.
We need to be moving this kind of code into user control. I think we have this kind of code in climatic data entry. I suggest you add a TODO comment.
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.
@N-thony I think the abbreviate works fine now. But your function has now (behind the scenes) changed the janitor function for clean names. I think it also applies the default width from the abbreviate function.
At least the result of the clean names, with snake works fine in version 0.7.4 and is now different in your current function. I was trying with the iris set.
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.
Great - this is now an excellent new feature. Hope @shadrackkibet or @lloyddewit can approve, so it can be merged
@N-thony I think there are 3 open comments from @shadrackkibet that are still open. Please could you fix these (without setting to resolved) and then ask @shadrackkibet to check. If he's happy, then he can resolve these remaining comments and approve. @shadrackkibet thanks for reviewing this PR, I don't think I need to review also, but if you'd like me to review, then let me know, thanks |
@lloyddewit I think the three comments were already solved. |
@N-thony I looked at the code and couldn't see where the comments are solved. Maybe I'm missing something. For each comment, please could you explain where/how the comment is solved? |
@N-thony thank you for the update |
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.
@rdstern please approve then we can merge.
Fixes (partially) #6877
This is still a work in progress. @shadrackkibet I have added the grid under the multiple options as suggested by @rdstern in the corresponding issue. At this stage, I would like your help on the backend side. I'm not sure about how the R code or proper package to use for renaming multiple variables through the grid.