Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
get_group_content()
lets you get content that groups can access #337feat:
get_group_content()
lets you get content that groups can access #337Changes from 6 commits
2fd27af
c83e624
0ffa3e3
f762c2a
e0a6bcc
e1525f7
856e198
1bab41c
cfb514d
ed9ef94
e3bc1ec
18953f9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Two new endpoints to support new group functionality. Makes me wonder if I should add a function to call the group details endpoint, to get the details from just the GUID. Probably not high priority for this PR.
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.
Is
group_details()
called anywhere?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.
It was previously, when I was looking up the group name for GUID-only invocations. But it's not now, so I've removed it.
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.
Two things: (1) we don't need to write this to
out
yeah? (2) We use implicit returns at the end of functions, right?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.
The first part of the file (
get_groups()
andget_group_members()
) were just moved verbatim fromget.R
. I think it's probably more helpful to organize the code consistently by subject domain, and to move away from the paradigm of having most of the "get this type of object" functions inget.R
.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.
New functions that comprise the implementations of the
get_group_content()
, factored out slightly for clarity of code and testability.Notes for reviewers here, which should probably become code comments:
get_group_content()
, handles input validation and normalization (to allow the use of both GUIDs and data frames), and callsget_group_content_impl()
across all input groups.get_group_content_impl()
gets the content for a single group from the guid and name. It isn't exported because it is the strictly less flexible core ofget_group_content()
.extract_role()
is factored out for unit tests. Probably also useful elsewhere.