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

Lines are split improperly with --enable-replacement #168

Open
AlexanderRichert-NOAA opened this issue Mar 1, 2024 · 3 comments
Open

Lines are split improperly with --enable-replacement #168

AlexanderRichert-NOAA opened this issue Mar 1, 2024 · 3 comments

Comments

@AlexanderRichert-NOAA
Copy link

AlexanderRichert-NOAA commented Mar 1, 2024

When split_reformatted_line splits lines, the incorrect indices are used when --enable-replacement has been applied, changing the number of characters in the line. The replacements are imposed by replace_relational_single_fline(f_line,... after get_linebreak_pos(lines,... has been called, so when format_single_fline is called in the impose_whitespace block, the line gets sliced in the wrong place(s).

Example:

$ cat test.F90
   IF(SCAN_MODE==64 .AND. IGDTMPL_THIN(9)==73 .AND. &
      IDLAT==IGDTMPL_THIN(18) .AND. (TEST1 .OR. TEST2) ) THEN
$ fprettify --enable-replacement test.F90 && cat test.F90
IF (SCAN_MODE .eq. 64 .AND. IGDTMPL_THIN(9) .eq. 73 .A &
ND. IDLAT .eq. IGDTMPL_THIN(18) .AND. (TEST1 .OR. TEST2)) THEN

The .AND. does not get split if I disable replacement, or if I replace the == with .EQ. myself (therefore making the number of characters in the line not change after replacement).

Update: I can work my way around this with the following change to __init__.py, though I have not extensively tested it:

@@ -1561,16 +1561,17 @@ def reformat_ffile_combined(infile, outfile, impose_indent=True, indent_size=3,
             lines, pre_ampersand, ampersand_sep = remove_pre_ampersands(
                 lines, is_special, orig_filename, stream.line_nr)
 
-            linebreak_pos = get_linebreak_pos(lines, filter_fypp=not indent_fypp)
-
             f_line = f_line.strip(' ')
 
             if impose_replacements:
                 f_line = replace_relational_single_fline(f_line, cstyle)
+                lines = [replace_relational_single_fline(l, cstyle) for l in lines]
 
             if impose_case:
                 f_line = replace_keywords_single_fline(f_line, case_dict)
 
+            linebreak_pos = get_linebreak_pos(lines, filter_fypp=not indent_fypp)
+
             if impose_whitespace:
@dbroemmel
Copy link
Contributor

I think that's similar to #153. And your change appears to be much shorter than my proposed solution, so I'd vote for that. I'd hope my proposed changed test still makes sense.

@AlexanderRichert-NOAA
Copy link
Author

Do you have a PR set up for your solution yet? If not, I can put one in, and then if you're willing to help contribute testing to that, that'd be great.

@dbroemmel
Copy link
Contributor

#154 is my uglier fix. The test is pretty basic, I've simply adjusted the existing one to catch this case as well I believe. But I've also seen in #127 that the tests are going to be changed?

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

No branches or pull requests

2 participants