-
Notifications
You must be signed in to change notification settings - Fork 23
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
Added globus auth scope show command #1077
base: main
Are you sure you want to change the base?
Conversation
Why? |
Because the launch of a We can reopen this debate, but let's not do it in this PR thread. |
Auth has confirmed their comfort with (1) this command structure proposal and (2) the decisions made in this PR that diverge our display slightly from their raw api. So this guy is open for business! |
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.
I only have a couple of minor comments I'd like us to evaluate before approval. I did a reread of the parts I had already reviewed and feel pretty good about the state of this.
MIN_COLUMNS = 6 | ||
|
||
def __init__(self) -> None: | ||
self._column_delta = 0 |
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.
May I suggest that we name this to align with indented()
as the indent? Also requires that we invert some of the arithmetic around it.
self._column_delta = 0 | |
self._current_indent = 0 |
I suggest this after a moment of confusion reading the +=
, -=
, etc during review.
"scope_id_or_string", | ||
( | ||
"24f3dcbe-7655-4721-bc64-d1c5d635b9a1", | ||
"https://auth.globus.org/scopes/actions.globus.org/hello_world", | ||
), | ||
) | ||
def test_show_scope(run_line, scope_id_or_string): | ||
load_response_set("cli.scopes") | ||
|
||
result = run_line(f"globus auth scope show {scope_id_or_string}") |
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.
I notice that we have pytest params matched to the data here. I think it's fine but if you wanted to break the dependency up a bit, you can
@pytest.mark.parametrize("lookup_method", ("id", "scope_string"))
def test_...:
rset = load_response_set("cli.scopes")
if lookup_method == "id":
scope_arg = rset.metadata["hello_world_id"]
elif lookup_method == "scope_string":
scope_arg = rset.metadata["hello_world_string"]
else:
raise NotImplementedError(lookup_method)
It's totally optional, but if you'd like to do so, there's a path to doing this and potentially something similar to the expected output formulation.
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 doesn't look like a conditional is required.
@pytest.mark.parametrize(
"metadata_key",
(
pytest.param("hello_world_id", id="lookup by scope id"),
pytest.param("hello_world_string", id="lookup by scope string"),
)
)
def test_...:
rset = load_response_set("cli.scopes")
scope_arg = rset.metadata[metadata_key]
self, | ||
auth_client: globus_sdk.AuthClient, | ||
scope_ids: list[str], | ||
default: str | None = "UNKNOWN", |
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.
Since None
isn't the default, I'm surprised that it appears in the type annotation. Is this an artifact of a previous rev?
if not value: | ||
return "[]" | ||
|
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 looks like this condition exists in two places.
First it appears here, which appears to break the past behavior (render([])
return ""
before but now returns "[]"
).
But then it's recreated in the subclass, which explicitly checks to see if the list is empty and then calls this function to rely on its behavior.
It seems like the subclass could take the responsibility to return "[]"
, rather than changing the behavior here. Is that accurate? Or am I overlooking some additional context?
A dictionary with support for lazily loaded values. | ||
Lazy loaders are registered using the `register_loader` method. | ||
|
||
Note: This class extends `dict`, not `UserDict` for json serializability. |
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.
I haven't seen UserDict
since the Python 2 days, when it was impossible to subclass the built-in dict
. I didn't realize it was still around!
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.
Probably the comment reflects some of my influence via code I've left around. 😁
We have it in the SDK as well, as the base for the payload classes!
I think UserDict
has some differences vs subclassing dict
regarding the consistency of various mutators. It's all very niche stuff and I don't think there's much call for UserDict
these days (and less for UserList
and less than that for UserString
!).
I also think it's incidentally nice that self.data
is available on UserDict
, since it means you can implement __getitem__
in terms of self.data.__getitem__
and have a clearer flow of control. So there is at least some rationale for still liking it!
But, of course, I'm not particularly attached to one implementation vs another for this sort of thing. 🙂
"scope_id_or_string", | ||
( | ||
"24f3dcbe-7655-4721-bc64-d1c5d635b9a1", | ||
"https://auth.globus.org/scopes/actions.globus.org/hello_world", | ||
), | ||
) | ||
def test_show_scope(run_line, scope_id_or_string): | ||
load_response_set("cli.scopes") | ||
|
||
result = run_line(f"globus auth scope show {scope_id_or_string}") |
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 doesn't look like a conditional is required.
@pytest.mark.parametrize(
"metadata_key",
(
pytest.param("hello_world_id", id="lookup by scope id"),
pytest.param("hello_world_string", id="lookup by scope string"),
)
)
def test_...:
rset = load_response_set("cli.scopes")
scope_arg = rset.metadata[metadata_key]
Added
globus auth scope show SCOPE_ID_OR_STRING
globus auth
is a hidden command tree; Stephen and I discussed keeping it that way until we have 5+ commands registered under there.ArrayMultilineFormatter(ArrayFormatter)
- Supports multiline formatted elements, adding a yaml-style-
element start indicator to the beginning of each new one.RecordFormatter
- Prints an object using theRecordPrinter
into a buffered string.LazyDict
- A dict with "key loaders" which will load values for specific keys when accessed the first time, caching the result.terminal_info
with a global singletonTERM_INFO
to centrally track and mutate content width.Changed
[]
instead of nothing.DraftThis PR is in draft until:I have a chance to put the command in front of the auth team.I figure out how to figure out reserved_width in RecordFormatter; it's a pretty minute thing but is hardcoded right now.Auth team has signed off in slack & I figured it out!
Usage
By Scope ID
Standard Output (dependent scope strings get loaded in a secondary batched call)
JSON Output (no secondary dependent scope resolution call happens)
By Scope String
Similarly when accessing by scope string
JSON by scope string uses a the get-scopes route so response is a list of one element