Skip to content

Implement support for basic LR and paginated LR#3

Open
josephschorr wants to merge 1 commit intomainfrom
basic-lr
Open

Implement support for basic LR and paginated LR#3
josephschorr wants to merge 1 commit intomainfrom
basic-lr

Conversation

@josephschorr
Copy link
Member

No description provided.

@josephschorr josephschorr requested a review from jzelinskie May 27, 2025 20:53

// LookupResources performs a lookup for resources that the subject has the
// provided permission on.
func (c *Client) LookupResources(
Copy link
Member

Choose a reason for hiding this comment

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

Is there ever a time you'd want to use this method if the paginated method exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah; if you want to control the pagination manually

Copy link
Member

Choose a reason for hiding this comment

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

Why not follow the pattern where you only have one function that works like this:

...
// Apply sane defaults like basic pagination.
for _, opt := range defaultLROpts() {
    opt(req)
}

// Override the defaults with anything provided by the caller.
for _, opt := range opts {
    opt(req)
}
...

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I want people to know when reading the code that one is paginated automatically

Copy link
Member

@jzelinskie jzelinskie May 29, 2025

Choose a reason for hiding this comment

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

I don't want people to have to think about pagination at all unless they want to, haha. By defaulting it on, they avoid having to learn pagination even exists and trying to pick a sane default value, instead that's done for them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, and they should use the paginated call for that. However, there will be times to not use it, and having the escape hatch is important

Copy link
Member

Choose a reason for hiding this comment

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

Why wouldn't they be able to do that by also having an option for it?

m.Run()
}

func ExampleClient_FilterRelationships() {
Copy link
Member

Choose a reason for hiding this comment

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

This was an intentional example that was for documentation. Can you add it back?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its still there, just as a test now

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer it be an example, it's not really designed to exercise much, instead show off how to use the API.


type Object struct {
Typ string
Type string
Copy link
Member

Choose a reason for hiding this comment

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

is this fully safe? i was avoiding the keyword

Copy link
Member Author

Choose a reason for hiding this comment

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

Why wouldn't it be? Its capitalized

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