-
Notifications
You must be signed in to change notification settings - Fork 35
Update namespaces in architecture to domain-based #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
Conversation
Reorganized classes into more specific namespaces to match #21. Moved message-related classes, file types, models, and enums into their appropriate sub-namespaces to improve code organization and follow single responsibility principle.
|
@borkweb Regarding #21, are we just waiting for @felixarntz's response to #21 (comment)? |
|
I was going to wait for the 👍 from both of you. There's also the question at the bottom of that regarding prefixes and suffixes that are a bit redundant. I'm not sure how y'all feel about that piece. |
|
I left some feedback in #21, let's iron that out first. Then we can update this PR as needed and review the details. |
JasonTheAdams
left a comment
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.
Looking good!
felixarntz
left a comment
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.
docs/ARCHITECTURE.md
Outdated
| namespace AiClient.Providers.Models.Enums { | ||
| class ModalityEnum { | ||
| TEXT | ||
| DOCUMENT | ||
| IMAGE | ||
| AUDIO | ||
| VIDEO | ||
| } | ||
| class FinishReasonEnum { | ||
| STOP | ||
| LENGTH | ||
| CONTENT_FILTER | ||
| TOOL_CALLS | ||
| ERROR | ||
| } | ||
| class OperationStateEnum { | ||
| STARTING | ||
| PROCESSING | ||
| SUCCEEDED | ||
| FAILED | ||
| CANCELED | ||
| } | ||
| } |
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's a good mental cross-check to see that this namespace currently remains relevant for the Implementer API (as this class diagram is only that part).
If we are going to be strict about it, we should probably move this to another namespace outside of AiClient.Providers. Since modalities are definitely relevant for implementers (as they can directly reference it when creating a prompt), and FinishReason belongs to Result, and OperationStateEnum belongs to Operation, I'd say it makes sense to move this to other Enums namespaces outside of AiClient.Providers.
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.
Finding a home for ModalityEnum is a little challenging. It feels like Models/Enums is the right home, but we don't/won't have a top level Models/ directory. So...I plopped it in a top level Enums/ directory.
Since this is the last holdout of this body of work, if you have an idea that makes sense - definitely push a commit to this PR or merge if we're good here 🕺
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 had a thought this morning! What if we did this:
- Make a
Promptsnamespace - Move the
PromptBuildertoPrompts - Move the
MessagesBuildersto Messages` - Put the common enums we want folks to be thinking of for prompts into the
Promptsdomain
That, or we move Enums to Builders.Enums. In any case, in keeping with the implementors thinking, we move the prompt builder and its commonly related parts close together.
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 like bullets 1-4 as an approach. I think it - at the very least - makes PromptBuilder more findable as Builders isn't as intuitive of a directory name. It also makes sense that the MessageBuilder would exist in Messages.
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.
Hmm, I see how finding a home for ModalityEnum isn't as straightforward as the other two (which I see you already migrated, and LGTM 👍 ).
But I don't think we should add a Prompts namespace, primarily because a "prompt" is not an actual thing in this SDK. There's no Prompt class for example, and the only thing that uses the term is PromptBuilder. To me, "prompt" is the higher-level term for whatever is sent to the model altogether - but if we made that a namespace, it would mean almost every other root level namespace could fall under it, which I find confusing.
IMO, ModalityEnum fits best under AiClientNamespace.Messages.Enums. Every Message consists of parts of different modalities, and that applies to both prompts as well as responses/results (for both of which Message is used).
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 think a case can be made in either direction, tbh. But ultimately, I'm not too picky and can get behind either option, so let's go with @felixarntz 🎈
…plementer API area. Resolve alphabetization issues. Replicate Tools in both Implementer and Extender areas
felixarntz
left a comment
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.
Except for the point on ModalityEnum, this LGTM.
I left some feedback on that comment thread. The more I think about it, the more intuitive I find for this to be under the Messages namespace.
felixarntz
left a comment
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.
This PR is in draft mode while we contemplate #21.
Reorganized classes into more specific namespaces to match #21. Moved message-related classes, file types, models, and enums into their appropriate sub-namespaces to improve code organization and follow single responsibility principle.
You can see the ARCHITECTURE.md here.