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

rearrangment/reformat of the source code #5

Closed
wants to merge 13 commits into from

Conversation

nbehrnd
Copy link
Contributor

@nbehrnd nbehrnd commented Aug 7, 2023

To ease contributions to the project, a style which can be checked by fprettify
(see equally style file fprettify.rc) is proposed.

To ease a consistent representation of the code, a style sheet for
fprettify was added.

Signed-off-by: Norwid Behrnd <[email protected]>
Signed-off-by: Norwid Behrnd <[email protected]>
Signed-off-by: Norwid Behrnd <[email protected]>
Signed-off-by: Norwid Behrnd <[email protected]>
The implmentation of the style requires a couple of manual edits
fprettify can not provide.  This commit saves an intermediate.

Signed-off-by: Norwid Behrnd <[email protected]>
Signed-off-by: Norwid Behrnd <[email protected]>
By application of the fprettify style, fences of procedures now are
dedented.  Simultaneously, they stay within the 80 character/line
limit.

Signed-off-by: Norwid Behrnd <[email protected]>
Manually, lines the fprettify style considered overly long are
split with `&`.  Eventually, all suggestions are considered, and
`fpm build` still successfully works without report of an error.

Signed-off-by: Norwid Behrnd <[email protected]>
The type definition now lists the procedures in an alphabetic sort.
If ford's documentation is not accessed, then this place is the
users' entry to get familiar with the procedures provided.

Signed-off-by: Norwid Behrnd <[email protected]>
To match the listing in the type block, the procedures equally are
sort alphabetically.  This work has started, still is incomplete.

Signed-off-by: Norwid Behrnd <[email protected]>
The alphabetic sort of the procedures is complete.  Their sequence
matches the listing in the type block.

Signed-off-by: Norwid Behrnd <[email protected]>
Correction of a minor typo, passing again fprettify checker.

Signed-off-by: Norwid Behrnd <[email protected]>
A brief note to use snake_case and the fprettify style for a
consistent code format was added.

Signed-off-by: Norwid Behrnd <[email protected]>
@nbehrnd
Copy link
Contributor Author

nbehrnd commented Aug 7, 2023

There are separate subroutines set_height and set_width about the images, naturally, allocate_pixels uses both of them. In review of the changes proposed above, I start to like the idea set_height and set_width could coalesce into one subroutine in common (e.g., set_dimensions) which manages both parameters in a single array in one place only.

What do you think about this?

@gha3mi
Copy link
Owner

gha3mi commented Aug 7, 2023

Thanks @nbehrnd

Personally, I use VSCode as my code editor with the Modern Fortran Extension. Modern Fortran, by default, uses findent for formatting. However, it is also possible to use fprettify within this extension. I haven't modified the default formatting settings, but I am open to using your suggested formatting style. (This is my personal idea, but we can discuss it further: we could apply a formatting process during the review stage after a pull request (PR) is sent. This way, developers can focus on coding, and during the review process, we can suggest or implement the desired formatting style.)

Sorting all procedures alphabetically can be a bit difficult in the future. If the name of a subroutine changes, then the position of the subroutine will also change.

Regarding the set_dimension procedure, I believe it is possible to add a new subroutine for set_dimension without removing set_height and set_width. This gives users or developers the ability to set only height or width if needed in the future. Alternatively, we could create a set_dimension subroutine with optional height and width parameters.

 elemental pure subroutine set_dimension(this, height, width)
    class(format_pnm), intent(inout) :: this
    integer, intent(in), optional :: height
    integer, intent(in), optional :: width

    if (present(height)) this%height = height
    if (present(width)) this%width = width
 end subroutine set_dimension

To discuss all topics separately, please open an issue for each topic. This way, we can discuss each topic individually before sending a PR and merging it into the main branch.

@nbehrnd
Copy link
Contributor Author

nbehrnd commented Aug 8, 2023

@gha3mi My suggestion of fprettify is biased because of an encounter in ABIN as file .fprettify.rc where the explicit list of all parameter (levels) aim to provide the same result regardless if defaults of fprettify would change. And a PR (fortran-lang/fprettify#146) to fprettify as initiated by Patrick Seewald. (I don't know how much he is envolved in other projects on GitHub where this string appears in name.) This being said, because findent nicely works from the CLI, I'm fine to use it here instead. From a test on forimage.f90 as currently in branch main, it looks like you run it with the defaults of parameters; ok. Because of partially clipped lines in html pages generated by ford (e.g., about example1.f90), though one can use a slider to access the initially invisible part, I'm still in favour of a (manual) line break after 80 characters.

The alphabetic sort of the procedures was a brief sort by vim of the lines in the type block marked with subsequent copy-yank of the procedures per sé below for consistency. With the earlier listing in hand (in the type block), however, easy enough to pick up the procedure's name and to retrieve it further down in the module.

I now understand the separate management of set_height and set_width as the safer approach than both parameters in one common procedure (alone) and hence retract the idea to coalesce the two.

@nbehrnd nbehrnd closed this Aug 8, 2023
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.

2 participants