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

Fixes and new options #99

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Jellby
Copy link

@Jellby Jellby commented Feb 9, 2021

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.

@pseewald
Copy link
Collaborator

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.

@Jellby
Copy link
Author

Jellby commented Feb 10, 2021

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?

@pseewald
Copy link
Collaborator

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.

@Jellby
Copy link
Author

Jellby commented May 24, 2021

Anything else I should do?

@foxtran
Copy link

foxtran commented Sep 14, 2021

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

@Jellby
Copy link
Author

Jellby commented Sep 14, 2021

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

@pseewald pseewald added bug non-critical bug (or with workaround) enhancement Enhancement to existing feature labels Aug 30, 2022
@pseewald pseewald linked an issue Aug 30, 2022 that may be closed by this pull request
3 tasks
@stigh
Copy link

stigh commented Feb 29, 2024

@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 #163). But I still suggest to merged it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug non-critical bug (or with workaround) enhancement Enhancement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wish for more fine-grained options
4 participants