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

Dollar sign within quotations fix #151

Merged
merged 3 commits into from
Dec 19, 2024
Merged

Conversation

e-perl-NOAA
Copy link
Contributor

See issue #150.

A fix in rm_dollar_sign.R to change text()$text within quotes (which would normally throw an error because it converted it to text()[["text"]]) to text()[['text']] instead, thereby fixing the issue of multiple double quotes in a line.

Tested locally by using the r4ss function file (get_ss3_exe.r) that originally made me aware of this issue:
https://github.com/r4ss/r4ss/blob/bb679f422ef6aa9189fe99c14375bee00dab2fa1/R/get_ss3_exe.r#L87

To test via GitHub actions I reverted the fixed line in the main branch of r4ss which has curl::curl_version()[['ssl_version']] back to curl::curl_version()$ssl_version and then ran the updated rm_dollar_sign.R function and had a successful run here.

@e-perl-NOAA e-perl-NOAA added the bug Something isn't working label Dec 18, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 9 lines in your changes missing coverage. Please review.

Project coverage is 93.36%. Comparing base (499d1d5) to head (58c37b3).

Files with missing lines Patch % Lines
R/rm_dollar_sign.R 72.72% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
- Coverage   95.19%   93.36%   -1.83%     
==========================================
  Files           4        4              
  Lines         208      226      +18     
==========================================
+ Hits          198      211      +13     
- Misses         10       15       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@k-doering-NOAA
Copy link
Collaborator

Thanks so much @e-perl-NOAA!! Working on reviewing this now!

@k-doering-NOAA k-doering-NOAA removed the request for review from Bai-Li-NOAA December 19, 2024 17:09
Copy link
Collaborator

@k-doering-NOAA k-doering-NOAA left a comment

Choose a reason for hiding this comment

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

Thanks so much @e-perl-NOAA, this is a great fix!!! I added a test to test that to make sure it works and it does!

However, I did realized we didn't account for nested lists in quotes, e.g., "list$item1$item2 in quotes", so I added some additional code; could you see if that looks good to you (I did add a test for this scenariom too)? If it looks good, I think we can merge this in!

@e-perl-NOAA
Copy link
Contributor Author

Just tested the nested portion locally and it works for me!

@e-perl-NOAA
Copy link
Contributor Author

I'll let you merge it in!

@k-doering-NOAA k-doering-NOAA merged commit 4562601 into main Dec 19, 2024
11 checks passed
@k-doering-NOAA k-doering-NOAA deleted the quotes_dollar_sign_fix branch December 19, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants