-
Notifications
You must be signed in to change notification settings - Fork 30
[r] Add functions for setting/getting global context #4364
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4364 +/- ##
==========================================
- Coverage 86.62% 85.36% -1.26%
==========================================
Files 139 77 -62
Lines 20955 13814 -7141
Branches 15 15
==========================================
- Hits 18152 11793 -6359
+ Misses 2803 2021 -782
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
aaronwolen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Great documentation and user messaging.
There seems to be a lot of overlap between the getter/setter's functionality. It looks like both functions:
- Check if a context exists
- Create one if needed
- Store it in
.pkgenv[["somactx"]] - Return the context
So the only differences are:
set_default_context()acceptsconfigand always creates a new oneget_default_context()has no args and only creates one if missing
I'd expect get_default_context() to error if one wasn't set and suggest users call set_default_context(). That might make the use case for each function clearer.
And then for internal use within get_soma_context() maybe we add an internal helper .get_or_create_default_context() that matches the current behavior of get_default_context() so factory functions automatically handle lazy creation for users.
f7693c6 to
2c6a0a0
Compare
915bae4 to
090d452
Compare
aaronwolen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! As discussed, a took a stab at expanding the docs since this concept may be unfamiliar to some R users. Also suggested updating the set_default_context() to invisibly return the context object, which is typical for setters.
Co-authored-by: Aaron Wolen <[email protected]>
Co-authored-by: Aaron Wolen <[email protected]>
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-2.3 release-2.3
# Navigate to the new working tree
cd .worktrees/backport-release-2.3
# Create a new branch
git switch --create backport-4364-to-release-2.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 671a49fd4c1dd808f212efb4fb85541c8a1711a1
# Push it to GitHub
git push --set-upstream origin backport-4364-to-release-2.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-2.3Then, create a pull request where the |
* Add functions to set/get global default context * Update NEWS, docs, and version number * Add export to function * Update NEWS * Fix typo in check * Update get_context_default and docs * Fix typo * Add `replace` option to context and format file * Update docs and comment out deprecation test * Apply suggested documentation improvements Co-authored-by: Aaron Wolen <[email protected]> * Update documentation * Apply suggestions from code review Co-authored-by: Aaron Wolen <[email protected]> * Update documentation --------- Co-authored-by: Aaron Wolen <[email protected]> (cherry picked from commit 671a49f)
* Add functions to set/get global default context * Update NEWS, docs, and version number * Add export to function * Update NEWS * Fix typo in check * Update get_context_default and docs * Fix typo * Add `replace` option to context and format file * Update docs and comment out deprecation test * Apply suggested documentation improvements * Update documentation * Apply suggestions from code review * Update documentation --------- (cherry picked from commit 671a49f) Co-authored-by: Aaron Wolen <[email protected]>
Add a method for setting the global context and one for getting the global context. Use the one to get the global context in internal context helper function.