Skip to content

Replace save/load with saveRDS/readRDS to avoid environment side effects#3810

Closed
Thomas-Sedhom wants to merge 4 commits into
PecanProject:developfrom
Thomas-Sedhom:saveRDS-refactor
Closed

Replace save/load with saveRDS/readRDS to avoid environment side effects#3810
Thomas-Sedhom wants to merge 4 commits into
PecanProject:developfrom
Thomas-Sedhom:saveRDS-refactor

Conversation

@Thomas-Sedhom
Copy link
Copy Markdown
Contributor

Replace save/load with saveRDS/readRDS to avoid environment side effects

Description

The current codebase uses save() and load() for intermediate objects. This approach has several drawbacks:
1- save() requires creating temporary variables to store list elements which is awkward and error prone.
2- load() injects objects by name into the environment and potentially overwriting existing variables silently.
3- Reading specific objects requires extra gymnastics with custom environments.

and the solution is:
All problematic uses of save() and load() have been converted to saveRDS() and readRDS():
1- saveRDS() saves the object directly, without relying on its name.
2- readRDS() explicitly returns the object, which can be assigned to any variable.
This eliminates side effects and makes the code more readable and maintainable.

Motivation and Context

This pattern makes the code harder to maintain and debug.

#3735

Review Time Estimate

  • Immediately
  • Within one week
  • When 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.

@infotroph
Copy link
Copy Markdown
Member

@Thomas-Sedhom Thanks for taking this on! You're taking the right direction for what was originally suggested in #3735, but seeing the diff makes me realize there's a backwards compatibility angle that needs some attention. I said more in the issue thread -- Let's discuss there and then come back here to implement once the design is right.

@infotroph
Copy link
Copy Markdown
Member

Closing this because of substantial overlap with a bigger refactor @omkarrr2533 is leading. Thanks for the contribution and sorry it went unused!

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.

2 participants