Skip to content
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: add 'Viewer' class disentangled from 'User' #48

Merged
merged 14 commits into from
Jul 31, 2024
Merged

Conversation

bmschmidt
Copy link
Collaborator

@bmschmidt bmschmidt commented May 31, 2024

This PR disentangles two concepts in the module:

  1. The AtlasUser in the system, which is a set of information like an e-mail, a name, a profile picture, etc.
  2. The basket of permissions that we hit the API endpoint with.

Fusing them together leads to a monstrous solipsism where the AtlasUser cannot imagine any user other than himself. He cannot collaborate with other AtlasUsers unless they give him full access to their APIKeys and authority to hit endpoints with them.

Following practice at Meta, we introduce a new fundamental class called the AtlasViewer, which is the agent making requests. AtlasUser is just the person in the system. For the time being AtlasUser still accepts keys in its constructor, but it passes them through to the underlying AtlasViewer: this behavior will probably be deprecated in version 1.0.


🚀 This description was created by Ellipsis for commit 325def1

Summary:

Refactored the SDK to introduce AtlasViewer for handling API requests, separating it from AtlasUser which now only represents user information.

Key points:

  • Introduced AtlasViewer class in src/viewer.ts to handle API requests.
  • Updated AtlasUser class in src/user.ts to represent user information only.
  • Modified constructors and methods in src/embedding.ts, src/index.ts, src/organization.ts, src/project.ts, and src/projection.ts to use AtlasViewer for API calls.
  • Updated type definitions in src/global.d.ts to include AtlasViewer related types.
  • Adjusted imports and exports in src/main.ts to include AtlasViewer.

Generated with ❤️ by ellipsis.dev

@bmschmidt bmschmidt requested review from RLesser and 4nalog May 31, 2024 14:56
Copy link
Collaborator

@RLesser RLesser left a comment

Choose a reason for hiding this comment

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

This looks good, the decoupling of viewer and user seems correct.

@bmschmidt
Copy link
Collaborator Author

@ellipses-dev review this

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to f5880d4 in 3 minutes and 0 seconds

More details
  • Looked at 8789 lines of code in 15 files
  • Skipped 3 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. src/index.ts:20
  • Draft comment:
    To ensure clarity and proper usage, it would be beneficial to update the constructor to only accept AtlasViewer as it aligns with the new design where AtlasUser is being phased out. This change would make the API more consistent and prevent potential errors from misuse.
constructor(id: Atlas.UUID, viewer: AtlasViewer, options: IndexInitializationOptions = {}) {
  super(viewer);
  ...
}
  • Reason this comment was not posted:
    Confidence of 30% on close inspection, compared to threshold of 70%.

Workflow ID: wflow_5LLpBa8MXQWbGoO2


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

6 days left in your free trial, upgrade for $20/seat/month or contact us.

@RLesser RLesser changed the base branch from nom-1545-formalize-info-caching-across-classes-the-same-way to main July 20, 2024 00:23
@RLesser RLesser changed the base branch from main to nom-1545-formalize-info-caching-across-classes-the-same-way July 20, 2024 00:23
@RLesser RLesser changed the base branch from nom-1545-formalize-info-caching-across-classes-the-same-way to main July 30, 2024 21:09
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on 3f0e196 in 32 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_yxdI8WThrJHXbtys


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

src/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@RLesser RLesser left a comment

Choose a reason for hiding this comment

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

Merged main in and settled merge conflicts. worth looking at the changes here again @bmschmidt before we merge in

@bmschmidt bmschmidt changed the title Viewer-refactor feat: add 'Viewer' class disentangled from 'User' Jul 31, 2024
@RLesser RLesser merged commit b55eff3 into main Jul 31, 2024
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