Skip to content

document packages #2316

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

Merged
merged 1 commit into from
Apr 15, 2025
Merged

document packages #2316

merged 1 commit into from
Apr 15, 2025

Conversation

miparnisari
Copy link
Contributor

@miparnisari miparnisari commented Apr 10, 2025

I glazed over the code to see what is implemented and to get a feeling of the overall organization. (And I fixed some typos along the way)

What stood out to me is that there are lot of folders with the same name within internal and within pkg. The line of what goes where seems blurry. E.g.:

  • graph (only 2 files in pkg?)
  • caveats. And, surprisingly, different Evaluation Results structs exist in both folders
  • namespace / schema

I believe that this could be simplified by merging folders, but this could break existing consumers of pkg if we move to internal. Who are the code consumers of pkg today? Do we provide guarantees on its stability?

Other things I noticed:

  • I think it'd be good to document in https://authzed.com/docs/spicedb/ what request and response headers are available.
  • the code used by the Playground appeared to be spread across a couple of folders

I'd be happy to submit follow-up PRs for all these things 😄

@github-actions github-actions bot added area/CLI Affects the command line area/datastore Affects the storage system area/dispatch Affects dispatching of requests labels Apr 10, 2025
@github-actions github-actions bot added the area/schema Affects the Schema Language label Apr 10, 2025
@@ -1,3 +1,4 @@
// Package developmentmembership defines operations with sets. To be used in tests only.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope i got this right... developmentmembership wasn't a clear name 😅

@miparnisari miparnisari marked this pull request as ready for review April 10, 2025 06:31
@miparnisari miparnisari requested a review from a team as a code owner April 10, 2025 06:31
@miparnisari miparnisari force-pushed the document-packages branch 2 times, most recently from f3b55ae to 3593d01 Compare April 10, 2025 16:47
@github-actions github-actions bot added the area/api http Affects the HTTP Gateway API label Apr 10, 2025
vroldanbet
vroldanbet previously approved these changes Apr 10, 2025
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

LGTM, see comments

@@ -44,7 +44,7 @@ func (ss SubjectSet) UnionWithSet(other SubjectSet) error {
return ss.BaseSubjectSet.UnionWithSet(other.BaseSubjectSet)
}

// WithParentCaveatExpression returns a copy of the subject set with the parent caveat expression applied
// WithParentCaveatExpression returns a copy of the subject set with the parent caveat expression applied
Copy link
Contributor

Choose a reason for hiding this comment

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

nbsp?

@@ -0,0 +1,2 @@
// Package servicespecific defines middleware that injects other middlewares.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Package servicespecific defines middleware that injects other middlewares.
// Package servicespecific defines middleware that injects other middlewares for service-specific use cases.

Comment on lines +1 to +2
// Package development contains code that runs in the Playground.
package development
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is also used by zed. Maybe "used by other libraries to facilitate schema development?"

@miparnisari miparnisari added this pull request to the merge queue Apr 15, 2025
Merged via the queue into main with commit 19a42cf Apr 15, 2025
41 checks passed
@miparnisari miparnisari deleted the document-packages branch April 15, 2025 20:36
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/api http Affects the HTTP Gateway API area/CLI Affects the command line area/datastore Affects the storage system area/dispatch Affects dispatching of requests area/schema Affects the Schema Language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants