fprettify icon indicating copy to clipboard operation
fprettify copied to clipboard

Lines are split improperly with --enable-replacement

Open AlexanderRichert-NOAA opened this issue 1 year ago • 3 comments
trafficstars

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:

AlexanderRichert-NOAA avatar Mar 01 '24 03:03 AlexanderRichert-NOAA

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.

dbroemmel avatar Apr 04 '24 11:04 dbroemmel

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.

AlexanderRichert-NOAA avatar Apr 04 '24 18:04 AlexanderRichert-NOAA

#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?

dbroemmel avatar Apr 05 '24 05:04 dbroemmel