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

ENH: Use findent to indent all .F90 files #366

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Patol75
Copy link
Contributor

@Patol75 Patol75 commented Nov 30, 2022

I have run into findent today, and I tried it on all .F90 source files within the Fluidity tree. The result lies within the branch associated with the present PR. Provided that CI returns green, would you say that such a change is desirable? Unfortunately, findent is not available as a pre-commit hook, but perhaps it will be in the future.

@gnikit
Copy link
Member

gnikit commented Nov 30, 2022

I will see if I can make a pre-commit hook today. FYI fprettify is another option although it will do more than just indentation.

@cianwilson
Copy link
Contributor

While I see that this change is aesthetically pleasing I'm curious what its benefits are beyond that.

The major downside appears to be that it will almost certainly make every branch conflict on merge. I'm already finding that in my branches with the existing hooks. So I'm curious what the upside is beyond aesthetics.

Thanks!

@gnikit
Copy link
Member

gnikit commented Dec 3, 2022

While I see that this change is aesthetically pleasing I'm curious what its benefits are beyond that.

findent can do some refactoring and Fixed -> Free form conversion, but I personally wouldn't use it for that. Fluidity is stable, so "if it ain't broke don't fix it" seems like a solid strategy.

The major downside appears to be that it will almost certainly make every branch conflict on merge.

Specifically for findent that shouldn't happen. Assuming we use findent for indentation and nothing else, the only diff from the base should be whitespaces, no new lines, no added code, which a simple rebase is able to resolve.

The only nontrivial downside of formatting the entire codebase would be that navigating git file history would be slightly more annoying. I personally don't mind since I use VS Code + GitLens which is a GUI, but I understand that the majority of the other devs are more terminal-driven in their development.

In general, aesthetic changes such as this, are by definition subjective and reinforced by personal preferences so I'm okay with whatever you decide to do with this PR.

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

Successfully merging this pull request may close these issues.

3 participants