Skip to content

For discussion: initial configuration of Codee Fortran formatter with examples (src/*.F90) #707

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.

Sign up for GitHub

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

Open
climbfuji wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

For discussion: initial configuration of Codee Fortran formatter with examples (src/*.F90) #707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b016c99
Add initial .codee-format and temporary run_codee_tmp.sh
climbfuji Dec 8, 2025
65d5c06
Codee format src/*.F90
climbfuji Dec 8, 2025
43778b8
Update format
climbfuji Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions .codee-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
# DH* 20251208 - this initial .codee-format is
# identical to the NEPTUNE version except for
# Comments: ! IndentIfAlreadyIndented # Indent
# (in NEPTUNE, we use "Indent")

# For a detailed description of all options, see:
# https://docs.codee.com/formatter/style-options

AlignAmpersandToColumnLimit: false
AlignAssignmentOperators: true
AlignUseItems:
Kind: OneItemPerLine
FirstLineFit: FitIfPossible
BreakBeforeBinaryOperators: true
Casing:
Identifiers: Lowercase # Preserve
Copy link
Collaborator Author

@climbfuji climbfuji Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NEPTUNE uses Lowercase, "Preserve" is the codee default

Copy link
Member

@dustinswales dustinswales Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Six of one, half of the other in the UFS.
My OCD is suggesting that we should adopt a convention and apply across UFS, but "Preserve" is fine for now

Copy link
Collaborator

@peverwhee peverwhee Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with whatever so perhaps lowercase is best to appease the NEPTUNE?

Copy link
Collaborator Author

@climbfuji climbfuji Dec 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase is nice, but whatever works for SIMA and the UFS will do. Within NEPTUNE, we had to make an exception for a nuopc subdirecty, because esmf/nuopc uses CamelCase extensively, and the notorious long names in esmf code become unreadable unless you add underscores. Long story short, no strong preference here.

Copy link
Collaborator

@gold2718 gold2718 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just for the Fortran bits of the framework? If so, we can just make sure it is readable in lowercase.

Copy link
Collaborator Author

@climbfuji climbfuji Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, only Fortran.

Keywords: Lowercase
LogicalConstants: Lowercase
LogicalOperators: Lowercase
RelationalOperators: Lowercase
UserDefinedOperators: Lowercase
ColumnLimit: 120
CommentDirectivePrefixes: []
DisabledDirectivePrefixes: []
IndentSize: 2
Copy link
Collaborator Author

@climbfuji climbfuji Dec 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be discussed

NEPTUNE and UFS use 2 spaces for indentation. The CCPP framework code in src uses 3 at the moment. This particular change is responsible for most of the differences in this PR.

Copy link
Collaborator

@gold2718 gold2718 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently have issues with lines that are too long so while this will not help a huge amount but it's at least in the correct direction.

# DH* No exception for TypeContains, FunctionContains etc
# requested 2025/12/08 using the Codee Online Form
IndentExceptions:
ModuleContains: IndentBeforeAndAfter
Comments: IndentIfAlreadyIndented # Indent
Copy link
Collaborator Author

@climbfuji climbfuji Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NEPTUNE is "Indent", here I am using "IndentIfAlreadyIndented"

Also note the comment above for ModuleContains exceptions and the missing support for TypeContains/FunctionContains etc exceptions.

Copy link
Member

@dustinswales dustinswales Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? I'd prefer "Indent" over "IndentIfAlreadyIndented".

Screenshot 2025-12-10 at 10 49 48 AM vs. Screenshot 2025-12-10 at 10 50 43 AM

Copy link
Collaborator Author

@climbfuji climbfuji Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because of the CCPP metadata hooks in the Fortran code that are typically not indented. I think capgen's parser is fine if they get indented, though.

Copy link
Collaborator

@gold2718 gold2718 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm for consistency so I prefer 'Indent'.

FixedFormLabelAlignment: Right
ContinuationIndentSize: DoubleIndentSize
DoubleColonSeparator: AddAlways
EndOfLineNormalization: Unix # Autodetect
Copy link
Collaborator Author

@climbfuji climbfuji Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unix is NEPTUNE, Autodetect is Codee default

Copy link
Member

@dustinswales dustinswales Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the documentation and don't fully understand this one?
(Maybe you or someone could explain (slowly) to me at out meeting tomorrow?)

Copy link
Collaborator Author

@climbfuji climbfuji Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to look this up, but as far as we all are concerned, Unix should be the correct choice.

Copy link
Collaborator Author

@climbfuji climbfuji Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually know if I've ever seen a case that mixes line endings in one file (as described in the codee documentation). If you use "auto-detect", it will use the first line ending across the entire file. We should definitely use "Unix" here so that all line endings, including Windows line endings for entire files/parts of files are converted to Unix.

Copy link
Collaborator

@gold2718 gold2718 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unix please!

EndStatementFormat: EndStructureAndName
EndStatementSeparation:
EndAssociate: Separated
EndBlockConstruct: Separated
EndBlockData: Separated
EndCritical: Separated
EndTeam: Separated
EndDoLoop: Separated
EndEnum: Separated
EndEnumerationType: Separated
EndForall: Separated
EndFunction: Separated
EndIf: Separated
EndInterface: Separated
EndModule: Separated
EndModuleProcedure: Separated
EndProgram: Separated
EndSelect: Separated
EndSubmodule: Separated
EndSubroutine: Separated
EndType: Separated
EndWhere: Separated
EnsureNewlineAtEOF: true
ConsecutiveEmptyLines:
MaxToKeep: 1
BetweenProcedures: 1
RemoveAtStartOfFile: true
RemoveAtEndOfFile: true
KindKeywordPrefix: AddAlways
# DH* TODO FILL THIS LIST
MacroIdentifiers: [
"__FILE__",
"__LINE__",
"_OPENMP",
]
RelationalOperators: UseSymbols
# DH* Note. Filed ticket 276 with Codee to prevent
# spaces between dimensions in dimension specifications
# like 'real, dimension(:,:,:), allocatable :: x'
SpacesAroundOperators:
LeftParenthesisExpression: NoTrailing
LeftParenthesisGeneric: NoSpaces
LeftParenthesisKeyword: OnlyLeading
RightParenthesisExpression: NoLeading
RightParenthesisGeneric: NoLeading
RightParenthesisKeyword: OnlyTrailing # NoLeading
Copy link
Collaborator Author

@climbfuji climbfuji Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OnlyTrailing is NEPTUNE, NoLeading is Codee default

Copy link
Member

@dustinswales dustinswales Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As expected, UFS is all over the place wrt "SpacesAroundOperators".
(I'm super impressed that NEPTUNE is following a standard!)

I do love the idea of adopting this (more granular?) level of code formatting.

Copy link
Collaborator

@gold2718 gold2718 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong feeling but squeezing spaces makes lines shorter.

Assignment: Both
Association: Both
ControlFlowAssignment: Both
KeywordAssignment: NoSpaces
ParameterAssignment: NoSpaces
BinaryArithmetic: Both
Exponentiation: NoSpaces
DefinedBinary: Both
DefinedUnary: NoTrailing
Relational: Both
RelationalLegacy: Both
LogicalBinary: Both
LogicalNot: NoTrailing
UnaryPlusMinus: NoTrailing
Comma: OnlyTrailing
Concat: Both
DoubleColon: Both
Comment on lines +95 to +97
Copy link
Collaborator

@peverwhee peverwhee Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the enforcement of Comma and DoubleColon white spacing?

We do this kind of thing (lining up intents, function arguments, etc) in SIMA quite often (which we prefer for ease of readability):

subroutine whatever(argument1, arg2, a3)
  integer,         intent(in) :: argument1
  real(kind_phys), intent(in) :: arg2
  integer,        intent(out) :: a3

Copy link
Collaborator Author

@climbfuji climbfuji Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a very good reason to not line up intents. If the longest line changes, then every single line has to change, even though there are no code changes. This increases the possibility of merge conflicts and make changes/pull requests unnecessarily longer. This is the reason why the code default is single whitespace, and why we've adopted it in NEPTUNE.

Copy link
Collaborator

@peverwhee peverwhee Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dustinswales @gold2718 do you have a preference on this?

Copy link
Collaborator

@gold2718 gold2718 Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I am sympathetic to the argument that the one-space rule minimizes potential merge conflicts and PR simplicity (one of the goals of these tools), at the end of the day, the code exists for its human users and they need to be able to read it to understand it. I know at NCAR, the users (scientists and RSEs) strongly preferred lining up the colons to make it easy to pick out the variables. Who is the customer here?

Copy link
Collaborator Author

@climbfuji climbfuji Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this today in the meeting; we agreed that aligning the colons doesn't need to be done for the auto-generated code, it's enough to do this for the few handwritten Fortran files in the repository. Thus, we just "preserve" the whitespaces on a per-file basis.

And importantly, host models can do whatever they want for the rest of their code ...

RemoveConsecutiveWhitespace: true
RemoveSemicolons: true
RemoveTrailingWhitespace: true
SeparateMultipleInlineStatements: true
19 changes: 19 additions & 0 deletions run_codee_tmp.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#!/usr/bin/env bash

files=(
"src/ccpp_constituent_prop_mod.F90:free"
Copy link
Member

@dustinswales dustinswales Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climbfuji Is the "free" suffix here to use "free formatted" version of Codee?

Copy link
Collaborator Author

@climbfuji climbfuji Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I had some logic in that script previously that would use this, it's no longer there because not needed.

Since the script won't be merged, I didn't bother taking this out.

"src/ccpp_hashable.F90:free"
"src/ccpp_hash_table.F90:free"
"src/ccpp_scheme_utils.F90:free"
"src/ccpp_types.F90:free"
)

for entry in "${files[@]}"; do
Copy link
Collaborator Author

@climbfuji climbfuji Dec 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is overly complicated and not really needed for ccpp-framework. Just for initial testing/demonstration purposes. This file will be either before this PR is merged, or at the latest once codee is integrated in GitHub actions.

Copy link
Member

@dustinswales dustinswales Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that we will pass any file modified in the current PR through the formatter as part of the PR?

Copy link
Collaborator Author

@climbfuji climbfuji Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion is to format all existing Fortran files in the repository as part of the PR. In a next step, implement GitHub actions. In a third step, integrate with capgen and make sure that the auto-generated code also complies with the formatting rules.

file="${entry%%:*}"
ext="${file##*.}"
fmt="${entry##*:}"
git checkout origin/develop -- $file
codee format --verbose --extensions=$ext --on-error force $file
echo ""
echo "-------------------------------------------------"
done
Loading