fortran-lang / fprettify

auto-formatter for modern fortran source code
https://pypi.python.org/pypi/fprettify
Other
368 stars 73 forks source link

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

Open AlexanderRichert-NOAA opened 6 months ago

AlexanderRichert-NOAA commented 6 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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?