Skip to content

refactor: Removes globalVariables()#1539

Open
kellijohnson-NOAA wants to merge 2 commits into
mainfrom
refactor-globalVariables
Open

refactor: Removes globalVariables()#1539
kellijohnson-NOAA wants to merge 2 commits into
mainfrom
refactor-globalVariables

Conversation

@kellijohnson-NOAA

@kellijohnson-NOAA kellijohnson-NOAA commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

What is the feature?

  • Use .data instead of globalVariables() to align with tidyverse best practices.

How have you implemented the solution?

  • import .data from rlang rather than ggplot2
  • align usage of .data throughout the codebase rather than a mix of .data and globalVariables()
  • added S3 class registration before FIMSFrame S4 class definition to tell S4 that tbl_df is a valid old-style S3 class

Does the PR impact any other area of the project, maybe another repo?

  • No

Disclaimer

Coding changes were made using codex.


Instructions for code reviewer

👋Hello reviewer👋, thank you for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete but feel free to skip over items that are not relevant!
  • See the GitHub documentation for how to comment on a PR to indicate where you have questions or changes are needed before approving the PR.
  • Please use standard conventional messages for both commit messages and comments
  • PR reviews are a great way to learn so feel free to share your tips and tricks. However, when suggesting changes to the PR that are optional please include nit: (for nitpicking) as the comment type. For example, nit: I prefer using a data.frame() instead of a matrix because ...
  • Engage with the developer. Make it clear when the PR is approved by selecting the approved status, and potentially commenting on the PR with something like This PR is now ready to be merged.

Checklist

  • The code is well-designed
  • The code is designed well for both users and developers
  • Code coverage remains high- [ ] Comments are clear, useful, and explain why instead of what
  • Code is appropriately documented (doxygen and roxygen)

@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.53917% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.41%. Comparing base (9448266) to head (76e4fe2).

Files with missing lines Patch % Lines
R/fimsframe.R 98.07% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1539      +/-   ##
==========================================
+ Coverage   87.39%   87.41%   +0.01%     
==========================================
  Files          97       97              
  Lines        8746     8757      +11     
  Branches      519      518       -1     
==========================================
+ Hits         7644     7655      +11     
+ Misses       1065     1064       -1     
- Partials       37       38       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ericward-noaa

Copy link
Copy Markdown
Contributor

These changes are relatively minor, but does a good job in making the code more consistent. I went through the changes that now use .data and all seem reasonable. I would highlight that this PR

  • increases the overall test coverage (only 1 line excluded in fimsframe)
  • doesn't import any additional packages
  • passing all tests (build_metrics probably just needs to be re-run )

@ericward-noaa ericward-noaa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review in comment on issue

@kellijohnson-NOAA

Copy link
Copy Markdown
Contributor Author

Thanks @ericward-noaa for reviewing this. The failed build metric is largely due to a tolerance that is too tight where the memory usage floats around at differences greater than 15% randomly, even if we only change a GitHub action line of code. I think we might need to revisit some of those tolerances now that we understand the runners a little bit better but we aren't entirely sure.

@kellijohnson-NOAA kellijohnson-NOAA force-pushed the refactor-globalVariables branch from 4791c05 to 76e4fe2 Compare June 18, 2026 13:47
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