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 three Replace options in the Find/Replace dialog #7484

Merged

Conversation

anastasia-mbithe
Copy link
Contributor

@anastasia-mbithe anastasia-mbithe commented May 19, 2022

Fixes (partially) #7304
This is still in progress. I am researching the function for the third option, replacing entire cell contents like it's done in Excel.

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.

I assume you have yet to implement the replace cell. The radio buttons look fine.
Perhaps check the code to update the default name of the replace cell option. If I go to the replace options it seems ok. Then if I go to the cell option, then back to detect (radio button at the top) and then back to replace, then it still has the name detect.

@anastasia-mbithe
Copy link
Contributor Author

@rdstern, I found a function to replace the cell, you can test it now. I also fixed the bug you referred to in your earlier comment.
It is ready for review.

@rdstern
Copy link
Collaborator

rdstern commented May 25, 2022

@anastasia-mbithe this seems a good start, but you are not yet checking carefully enough yourself first. Here is what I get, almost immediately:
image

Now I don't get the error if I add quotes round the Y, but that is not done for the other options here. And I don't think - even then - that you are checking for the Ignore Case. So you have a bit to go. I wonder what data you are using? I am using the example from rrefine to start with.

@anastasia-mbithe
Copy link
Contributor Author

@rdstern, I have changed the function for the cell button. This finds the pattern and replaces everything in the cell with the value specified. However, since the function finding the pattern "grepl()" is a regular expression function, I have disabled the Modifiers group box when the checkbox is checked. The function also supports ignoring case.

Kindly have a look at it.

rdstern
rdstern previously approved these changes Jun 2, 2022
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.

Looks good. I hope the code is also ok so it can be merged soon?

Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

@anastasia-mbithe Looks great, thank you.
I just renamed one of the variables.

@lloyddewit lloyddewit merged commit 7f95ca5 into IDEMSInternational:master Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants