Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Some cleanups and minor fixes #701

Merged
merged 11 commits into from
Jul 22, 2022
Merged

Conversation

zoziha
Copy link
Contributor

@zoziha zoziha commented May 27, 2022

Description

While using fpm and reading the fpm source code, I have found some small areas for improvement, as they tend to be small and have little impact on the overall functionality of fpm, and it doesn't seem worth it to list them as a single PR.

Updates

The work this PR does:

  1. Delete the unused fnv_1a reference;
  2. Update number_of_rows, remove unnecessary character reading;
  3. Update mkdir and os_delete_dir, the output format of which is somewhat complicated, and the current modification is unified with the following style:
    if(echo_local) print *, '+ ', cmd
  4. Logical operators using ==, /=, etc. are more intuitive and modern than .eq., .ne.;
  5. The display of compilation percentage output style has been corrected;
[  24%]Compiling ...  # Original
[ 24%] Compiling ...  # Now
  1. In the new_test test, the wildcard command line used in CMD shell on Windows did not actually take effect, which was corrected;

📢 PS. I made a single commit record for each fix, it would be simple enough to review them in turn.

@LKedward LKedward self-requested a review May 28, 2022 10:56
Copy link
Member

@LKedward LKedward left a comment

Choose a reason for hiding this comment

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

Thanks @zoziha - these changes all look fine to me. A couple of minor comments:

  • Are you able to fix this line in new-test as well:
    rm_command = 'rmdir ' // dirs_to_be_removed // ' /s /q'
  • In backend_output, perhaps we should just declare overall_progress with length 7 and remove the (:7) indexing?

@zoziha
Copy link
Contributor Author

zoziha commented May 28, 2022

Thanks for reviewing, @LKedward .

Sorry I didn't see this line of code before, I'll update my commit to fix it.

  • In backend_output, perhaps we should just declare overall_progress with length 7 and remove the (:7) indexing?

Nice suggestion ~

@zoziha zoziha force-pushed the package_by_package branch from cd8b5cc to 88834b4 Compare May 28, 2022 12:13
The wildcard command line usage on Windows to delete folders
does not take effect in the actual test.
@zoziha zoziha force-pushed the package_by_package branch from 88834b4 to 5253412 Compare May 28, 2022 12:15
Copy link
Member

@LKedward LKedward left a comment

Choose a reason for hiding this comment

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

Great stuff! Thanks @zoziha

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Thanks for sharing. Looks good to me.

@zoziha
Copy link
Contributor Author

zoziha commented Jul 22, 2022

Thanks you for the review, @LKedward , @awvwgk .

Since it got two approvals, I'll merge this PR.

@zoziha zoziha merged commit 3045817 into fortran-lang:main Jul 22, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants