(Closes #457) Fix bug with backslash in strings by arporter · Pull Request #462 · stfc/fparser

arporter · 2025-06-04T15:57:25Z

Weirdly, fparser was interpreting backslash "\" as an escape character. This is not a standard Fortran feature. i.e. if you want to escape a quotation mark in a Fortran string you simply repeat it: "''" means "'".

codecov · 2025-06-05T09:56:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.15%. Comparing base (77e0dd5) to head (279cce7).
Report is 19 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
- Coverage   92.15%   92.15%   -0.01%     
==========================================
  Files          86       86              
  Lines       13734    13733       -1     
==========================================
- Hits        12657    12656       -1     
  Misses       1077     1077

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

arporter · 2025-06-05T17:12:09Z

The PSyclone test suite is happy with this branch of fparser so I declare it ready for review. It is a small but quite fundamental change in behaviour that affects both fparser1 and fparser2. However, since interpreting a \ as an escape character is non-standard, I think this is OK.

hiker

Thanks a lot for this. The loop looks a lot simpler that what it was before, that's great. I do have the feeling that there is a bug though, please check the comments (and if I am wrong, it needs some tests :) ).

src/fparser/common/splitline.py

src/fparser/common/tests/test_splitline.py

arporter

Somehow I've accidentally started a review while attempting to reply to your comments. Anyhow, I've given up on AI and gone back to NI to write a new implementation. I think it's easier to understand now.

src/fparser/common/tests/test_splitline.py

src/fparser/common/splitline.py

arporter · 2025-06-25T15:14:13Z

OK, ready for another look now @hiker.

hiker

Ready to be merged, thanks!

src/fparser/common/splitline.py

src/fparser/common/tests/test_splitline.py

src/fparser/common/splitline.py

hiker · 2025-06-30T01:54:34Z

Dang, I need to take that "ready to be merged" back. One line was not covered by your new tests, and that line is actually wrong:
Test (continued line if no closing quote character is found). This adds two tests, but I believe they are actually the same (but the second tests again the handling of 'the other' quote character :) )

fparser/src/fparser/common/tests$ git diff ./test_splitline.py
diff --git a/src/fparser/common/tests/test_splitline.py b/src/fparser/common/tests/test_splitline.py
index cc155c5..68c2612 100644
--- a/src/fparser/common/tests/test_splitline.py
+++ b/src/fparser/common/tests/test_splitline.py
@@ -261,6 +261,8 @@ def test_splitquote(input_line, expected_parts, expected_unterm):
     "input_line, expected_parts, expected_unterm, stopchar, lower",
     [
         ("this is STILL a quote'", ["this is STILL a quote'"], None, "'", True),
+        ("this is STILL a quote", ["this is STILL a quote"], "'", "'", True),
+        ("this is STILL a quote\"", ["this is STILL a quote\""], "'", "'", True),
         ("'' STILL a quote'", ["'' STILL a quote'"], None, "'", True),
         ("'' STILL a', Quote", ["'' STILL a'", ", quote"], None, "'", True),
         ("'' STILL a', Quote", ["'' STILL a'", ", Quote"], None, "'", False),

I believe this is the fix:

fparser/src/fparser$ git diff common/splitline.py
diff --git a/src/fparser/common/splitline.py b/src/fparser/common/splitline.py
index f5667fa..bdcf55b 100644
--- a/src/fparser/common/splitline.py
+++ b/src/fparser/common/splitline.py
@@ -324,7 +324,7 @@ def splitquote(
             pos = end + 1
         else:
             # Didn't find a closing quotation char.
-            return String(line), stopchar
+            return [String(line)], stopchar
 
     while pos < n:
         start = _next_quote(line, start=pos)

arporter · 2025-07-14T14:54:09Z

Thanks for finding that bug. I didn't understand your suggested change to the test so have made my own. Hope it's in-line with what you had in mind. Weirdly, I couldn't find any report saying that line wasn't covered - how did you find it?

Ready for another look now.

…_concat

hiker

Looks good now. Your example tests that line similarly to mine, so that's fine.

arporter added 2 commits June 3, 2025 22:24

#457 simplify code identifying inline comment [skip ci]

9ae2ef4

#457 WIP adding copilot-generated implementation and tests [skip ci]

77e6f53

arporter self-assigned this Jun 4, 2025

arporter marked this pull request as draft June 4, 2025 15:57

#457 complete Copilot generation of new splitquote method

162ebb8

arporter mentioned this pull request Jun 4, 2025

Update 'runs on' target in CI and update black formatting #463

Closed

Merge branch 'master' into 457_str_concat

6c378e7

arporter added 4 commits June 5, 2025 11:03

#462 fix black formatting

70b6cf9

#457 revert unnecessary change and tidy test

7c62251

#457 put back test for coverage

274e4d8

#457 fix black formatting

d04ac5e

arporter marked this pull request as ready for review June 5, 2025 17:12

arporter requested review from LonelyCat124, hiker and sergisiso June 5, 2025 17:12

arporter added the ready for review label Jun 5, 2025

hiker added under review and removed ready for review labels Jun 23, 2025

hiker requested changes Jun 25, 2025

View reviewed changes

hiker added bug reviewed with actions PR has been reviewed and is back with developer and removed under review labels Jun 25, 2025

arporter added in progress and removed reviewed with actions PR has been reviewed and is back with developer labels Jun 25, 2025

arporter added 3 commits June 25, 2025 09:52

Merge branch 'master' into 457_str_concat

b024f80

#457 revert to NI instead of AI and re-implement

a310687

#457 improve docstring and test

9dcfe4e

arporter commented Jun 25, 2025

View reviewed changes

src/fparser/common/tests/test_splitline.py Show resolved Hide resolved

src/fparser/common/tests/test_splitline.py Show resolved Hide resolved

src/fparser/common/splitline.py Show resolved Hide resolved

#457 updates for review

4579407

arporter added ready for review and removed in progress labels Jun 25, 2025

hiker added under review and removed ready for review labels Jun 30, 2025

Merge remote-tracking branch 'origin/master' into 457_str_concat

1e8eb6e

hiker approved these changes Jun 30, 2025

View reviewed changes

src/fparser/common/splitline.py Show resolved Hide resolved

src/fparser/common/splitline.py Show resolved Hide resolved

src/fparser/common/tests/test_splitline.py Show resolved Hide resolved

src/fparser/common/splitline.py Outdated Show resolved Hide resolved

hiker added reviewed with actions PR has been reviewed and is back with developer and removed under review labels Jun 30, 2025

arporter and others added 2 commits July 14, 2025 15:41

Merge branch 'master' into 457_str_concat

8b0878c

#457 fix bug with continued, unclosed quote

a7f029e

arporter added ready for review and removed reviewed with actions PR has been reviewed and is back with developer labels Jul 14, 2025

Merge branch '457_str_concat' of github.com:stfc/fparser into 457_str…

1c65a0d

…_concat

hiker added the under review label Jul 21, 2025

hiker approved these changes Jul 21, 2025

View reviewed changes

#457 Updated changelog.

5dcd682

hiker added ready for merge PR is waiting on final CI checks before being merged. and removed ready for review under review labels Jul 21, 2025

#457 Fixed conflict.

279cce7

hiker merged commit c0e4626 into master Jul 21, 2025
5 checks passed

hiker deleted the 457_str_concat branch July 21, 2025 13:38

Conversation

arporter commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arporter commented Jun 5, 2025

Uh oh!

hiker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arporter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arporter commented Jun 25, 2025

Uh oh!

hiker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiker commented Jun 30, 2025

Uh oh!

arporter commented Jul 14, 2025

Uh oh!

hiker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arporter commented Jun 4, 2025 •

edited

Loading

codecov bot commented Jun 5, 2025 •

edited

Loading