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

Clean up clean command #948

Merged
merged 1 commit into from
Jun 22, 2023
Merged

Clean up clean command #948

merged 1 commit into from
Jun 22, 2023

Conversation

minhqdao
Copy link
Contributor

  • Remove some unused or unnecessary fields within fpm_clean_settings.
  • Some code cleanup and refactorings.

How to test

Verify that the functionality has not changed using the following commands:

  • fpm run -- clean
  • fpm run -- clean --skip
  • fpm run -- clean --all

Copy link
Contributor

@urbanjost urbanjost left a comment

Choose a reason for hiding this comment

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

Looks good. An optional suggestion might be to stop users from specifying both --all and --skip on the command line. in fpm_command_line.f90 in the case for 'clean' maybe a check like:

       if (all(specified(['skip','all '])))then
        call fpm_stop(6,'mutually exclusive options --all and --skip were specified on the clean subcommand')
    endif

and for consistency in the same section where the optional ampersand was added for a more consistent treatment on the CLEAN_CALL=LGET('all') line; should probably also add it in other places like just above that one the HELP_CLEAN, VERSION_TEXT) line as well. Maybe we should run all code through something like wfindent/findent, emacs, vim, fprettify, ... I dislike some
of the choices of findent, fprettify, ... line "end if" instead of "endif" and so on, but consistency would probably make things easier to read. Good with it as-is, so consider these comments for consideration. Using a standard format processor is probably a long-term consideration; I think it would be good to consider making --skip and --all mutually exclusive but users are unlikely to do it. Current behavior is that everything is removed if both are specified with no warning or error thrown.

@minhqdao
Copy link
Contributor Author

Thanks @urbanjost, I will add the mutual exclusion feature in another PR.

I'm also a big fan of automated code formatting. It has been proposed here #825 but it was dropped.

Thanks everyone for the reviews.

@minhqdao minhqdao merged commit ee397ac into main Jun 22, 2023
@minhqdao minhqdao deleted the clean-up-clean-command branch June 22, 2023 06:23
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.

4 participants