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

feat: Implement automated styling on .R files #3408

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

Conversation

Sweetdevil144
Copy link
Contributor

Optimize R Formatting Script to Target Only Staged .R Files

Fixes #3280 (Partially)
After reviewing discussion from #3280 and #3407 , I think one should take their time reviewing this. I will finish a review on each aspect of PEcAn to confirm if this script keeps the systems intact as they were previously.

Priority: Medium
Status: Ready for Review
Type: Enhancement

Description

I have optimized the existing R formatting script (format.sh) to improve its performance by targeting only the .R files that are staged for commit in the Git repository.

Key Changes:

  • Git Integration: After formatting, any modified files are automatically re-added to the staging area to ensure that the committed code adheres to the styling rules.
  • Error Handling: Enhanced error handling to skip files that no longer exist or cannot be accessed.

Motivation and Context

This optimization aligns with best practices for Git hooks by minimizing the impact on commit performance, thereby encouraging team members to maintain code quality without frustration.

This PR addresses where the formatting script was identified as a bottleneck in the commit process.

Review Time Estimate

  • Whenever possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I agree that PEcAn Project may distribute my contribution under any or all of
    • the same license as the existing code,
    • and/or the BSD 3-clause license.
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Additional Notes:

  • Documentation Update: While this change primarily affects the internal scripting for pre-commit hooks, I WILL updated the CHANGELOG.md to reflect the performance improvements if requested. If a more detailed documentation update is required, please let me know, and I can provide further details.

  • Compatibility: This optimization is backward-compatible and should not affect existing workflows other than improving performance.


@Sweetdevil144
Copy link
Contributor Author

Some roxygen errors persist. Facing errors while trying to install RTM package

echo "No changes made to: $file"
else
echo "Changes made to: $file. Re-adding to staging area."
git add "$file"
Copy link
Member

Choose a reason for hiding this comment

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

Styling can change Roxygen output as well, so may need to re-roxygenize the relevant package dir here too. Not sure offhand if in this context it makes more sense to do that via make or by directly invoking Roxygen.

Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

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

This will be easier to discuss, review, and get merged if you add the new scripts in one PR by itself and then do the initial styling of existing files in several separate PRs -- then we don't have to review >1200 files at once and can avoid creating merge conflicts with files touched by already-open PRs.

That said, from discussion to date in #3280 it's possible a pre-commit hook won't be the favored approach, so probably best to establish that before investing too many changes to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature : Automated linting for files
2 participants