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

Corrections to the rename dialog #9278

Merged
merged 4 commits into from
Nov 28, 2024

Conversation

Fidel365
Copy link
Contributor

@Fidel365 Fidel365 commented Nov 26, 2024

Fixes issue #9265 partially,
@rdstern , @N-thony , @lilyclements and @rachelkg this is ready for review

rdstern
rdstern previously approved these changes Nov 26, 2024
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fidel365 great, that is well done. There remains the additional question from @rachelkg, but I think that's answered in #8439. So I'll write in the issue.
Meantime, @N-thony another annoying bug solved, for you to check and merge.

Comment on lines 663 to 683
If rdoRenameWith.Checked Then
If rdoReplace.Checked Then
clsDefaultRFunction.AddParameter("type", Chr(34) & "rename_with" & Chr(34), iPosition:=1)
clsDefaultRFunction.AddParameter(".fn", "stringr::str_replace", iPosition:=2)
clsDefaultRFunction.AddParameter("pattern", Chr(34) & ucrInputReplace.GetText() & Chr(34), iPosition:=4)
clsDefaultRFunction.RemoveParameterByName("label")
clsDefaultRFunction.AddParameter("replacement", Chr(34) & ucrInputBy.GetText() & Chr(34), iPosition:=5)
If ucrInputEdit.GetText = "Starts With" Then
clsDefaultRFunction.AddParameter(".cols", clsRFunctionParameter:=clsStartwithFunction, iPosition:=3)
ElseIf ucrInputEdit.GetText = "Ends With" Then
clsDefaultRFunction.AddParameter(".cols", clsRFunctionParameter:=clsEndswithFunction, iPosition:=3)
ElseIf ucrInputEdit.GetText = "Matches" Then
clsDefaultRFunction.AddParameter(".cols", clsRFunctionParameter:=clsMatchesFunction, iPosition:=3)
ElseIf ucrInputEdit.GetText = "Contains" Then
clsDefaultRFunction.AddParameter(".cols", clsRFunctionParameter:=clsContainsFunction, iPosition:=3)
End If
Else
clsDefaultRFunction.RemoveParameterByName("pattern")
clsDefaultRFunction.RemoveParameterByName("replacement")
clsDefaultRFunction.RemoveParameterByName(".cols")
End If
Copy link
Collaborator

@N-thony N-thony Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we improve this as follow?

If rdoRenameWith.Checked AndAlso rdoReplace.Checked Then
    clsDefaultRFunction.AddParameter("type", Chr(34) & "rename_with" & Chr(34), iPosition:=1)
    clsDefaultRFunction.AddParameter(".fn", "stringr::str_replace", iPosition:=2)
    clsDefaultRFunction.AddParameter("pattern", Chr(34) & ucrInputReplace.GetText() & Chr(34), iPosition:=4)
    clsDefaultRFunction.RemoveParameterByName("label")
    clsDefaultRFunction.AddParameter("replacement", Chr(34) & ucrInputBy.GetText() & Chr(34), iPosition:=5)

    Select Case ucrInputEdit.GetText
        Case "Starts With"
            clsDefaultRFunction.AddParameter(".cols", clsRFunctionParameter:=clsStartwithFunction, iPosition:=3)
        Case "Ends With"
            clsDefaultRFunction.AddParameter(".cols", clsRFunctionParameter:=clsEndswithFunction, iPosition:=3)
        Case "Matches"
            clsDefaultRFunction.AddParameter(".cols", clsRFunctionParameter:=clsMatchesFunction, iPosition:=3)
        Case "Contains"
            clsDefaultRFunction.AddParameter(".cols", clsRFunctionParameter:=clsContainsFunction, iPosition:=3)
    End Select
Else
    ' Remove all related parameters if conditions are not met
    clsDefaultRFunction.RemoveParameterByName("pattern")
    clsDefaultRFunction.RemoveParameterByName("replacement")
    clsDefaultRFunction.RemoveParameterByName(".cols")
End If

@Fidel365
Copy link
Contributor Author

@N-thony I have made the changes requested, @rdstern I have made this Pr to partial since they are still further discussions on this on the issue.

Copy link
Collaborator

@N-thony N-thony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdstern over to you

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. We are having ambitious ideas about extensions, so I'm happy with the partially. But let's get this merged!
@N-thony you have approved, so just the merge to do.

@N-thony N-thony merged commit fb25373 into IDEMSInternational:master Nov 28, 2024
2 checks passed
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.

3 participants