fprettify icon indicating copy to clipboard operation
fprettify copied to clipboard

Fixes and new options

Open Jellby opened this issue 4 years ago • 7 comments

A couple of fixes:

  • else and else if statements can have a construct name at the end, just like end if (http://www.lahey.com/docs/lfprohelp/F95ARIFConst.htm)
  • Apparently initial indent (e.g. from fixed-format code) is removed if the first statement is program or module. Now it's also removed if it's subroutine or function. (Is an option needed for this behavor? What's the reason for this?)
  • Rearrange the number regex to not change the case of the kind when it's not an intrinsic (e.g. 1.0_Wp)
  • Do not crash when both --disable-indent and --disable-whitespace are passed

And new whitespace options:

  • --whitespace-end: add/remove space in end if, end do, etc. By default the same as --whitespace-intrinsics
  • --whitespace-only: add/remove space after the colon in use ... only: .... By default the same --whitespace-comma
  • --whitespace-if: add/remove space before the parenthesis in if (..., while (... and case (.... By default the same as --whitespace-intrinsics
  • --whitespace-do: add/remove space around the "assignment" in do statements. By default the same --whitespace-assignment
  • --whitespace-list: add/remove space after commas in list of variables of declarations and use statements (not in argument lists). By default the same as --whitespace-comma
  • --disable-indent-first: disable indent of the topmost level (similar to --disable-indent-mod, but also for subroutines and functions)
  • --indent-select: double indent of select blocks, such as e.g. case statements are indented once and the code inside is indented twice.

Jellby avatar Feb 09 '21 16:02 Jellby

Thanks a lot!

Apparently initial indent (e.g. from fixed-format code) is removed if the first statement is program or module. Now it's also removed if it's subroutine or function. (Is an option needed for this behavor? What's the reason for this?)

This goes back to pr #53 which I made default later on. There is no option to disable this as I thought that program / module should have 0 indent in any case. fprettify only supports free format. I don't agree with removing indent for subroutine / function by default, fprettify can also be applied to parts of a file (e.g. when used within an editor) and there removing indent for subroutine / function is not the expected behaviour. Do you reallly need this? If yes, could you make it an option?

--disable-indent-first: disable indent of the topmost level (similar to --disable-indent-mod, but also for subroutines and functions)

Do you really need this? I don't think this is a reasonable convention but I'd accept this option if you really see a use case for it.

Do not crash when both --disable-indent and --disable-whitespace are passed

I was not aware of this, thanks for catching and fixing this. I'll extend tests to this case.

Thanks a lot for the more fine-grained whitespace options which are very welcome! Would you have time to also add unit tests for the new options (just add new methods test_*, see this example? I just realized that for some reasons the automatic testing on travis CI does not work at the moment, I'll have a look.

pseewald avatar Feb 10 '21 12:02 pseewald

fprettify only supports free format.

The point was first make a fixed-format file free-format compatible (https://software.intel.com/content/www/us/en/develop/documentation/fortran-compiler-oneapi-dev-guide-and-reference/top/language-reference/program-elements-and-source-forms/source-forms/source-code-useable-for-all-source-forms.html) manually or with some other tool, and then use fprettify to, among other things, remove the unnecessary 6-column indent.

I don't agree with removing indent for subroutine / function by default, fprettify can also be applied to parts of a file (e.g. when used within an editor) and there removing indent for subroutine / function is not the expected behaviour. Do you reallly need this? If yes, could you make it an option?

Oh, I didn't think of applying fprettify to a partial file, then I agree on not doing it by default. But I have plenty of files that contain only a single subroutine/function, and I want to force it to 0 indent. I'll add an option (--reset-indent as in #53, or --whole-file, or whatever).

--disable-indent-first: disable indent of the topmost level (similar to --disable-indent-mod, but also for subroutines and functions)

Do you really need this? I don't think this is a reasonable convention but I'd accept this option if you really see a use case for it.

"Need" is a strong word, but I do want it. As above, when a file contains only a subroutine or function, it feels like a waste to indent every line. Isn't that the same reasoning behind --disable-indent-mod?

Jellby avatar Feb 10 '21 15:02 Jellby

Oh, I didn't think of applying fprettify to a partial file, then I agree on not doing it by default. But I have plenty of files that contain only a single subroutine/function, and I want to force it to 0 indent. I'll add an option (--reset-indent as in #53, or --whole-file, or whatever).

Ok, then you can reintroduce the option --reset-indent or name it --reset-indent-subroutine, as you prefer.

"Need" is a strong word, but I do want it. As above, when a file contains only a subroutine or function, it feels like a waste to indent every line. Isn't that the same reasoning behind --disable-indent-mod?

Yes it is and please go ahead if this feature is useful to you.

pseewald avatar Feb 10 '21 15:02 pseewald

Anything else I should do?

Jellby avatar May 24 '21 09:05 Jellby

@pseewald, @Jellby, any updates? @Jellby, could you please provide tests?

foxtran avatar Sep 14 '21 01:09 foxtran

What do you mean? There are some new tests in fprettify/tests/__init__.py.

Jellby avatar Sep 14 '21 07:09 Jellby

@gnikit, I think this PR looks good, and can be approved. I found --indent-select and disabling whitespace checks to be very useful.

Maybe this Do not crash when both --disable-indent and --disable-whitespace are passed could be improved, since it only avoids crashing, but still have no effect (ref https://github.com/pseewald/fprettify/issues/163). But I still suggest to merged it.

stigh avatar Feb 29 '24 12:02 stigh