[hold] refactor: Merge Active and Api code together #330
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
schloerke
changed the title
…rge_active_and_api_dict
| # TODO-barret-Q: Is this change correct? It use to be required, but the docs say not-required | ||
| id: NotRequired[str] |
There was a problem hiding this comment.
Is this change correct? It use to be required, but the docs say not-required
| # TODO-barret-future-q: Should this be inherited from ActiveFinderSequence? (It would add find_by) | ||
| def find(self) -> list[Association]: |
There was a problem hiding this comment.
Should this be inherited from ActiveFinderSequence? (It would add find_by) . We could have .find() and .find_by() be separated out into two ActiveFinder* classes.
| # TODO-barret-future-q; Should this auto retrieve? If so, it should inherit from ActiveSequence | ||
| class Integrations(ApiCallMixin, ContextP[Context]): |
There was a problem hiding this comment.
I'm really enjoying the clarity of when data is retrieved from .find() / .get() / .all() methods. And it does take the ambiguity out of when data is being retrieved.
I'd be up for dropping the "download all list-like data on initialization" in favor of the retrieval methods (e.g. .find() etc.) . This would also mean that the classes wouldn't be list-like would require the user to retrieve before calling any list like methods (ex: len(jobs.all()) and not len(jobs) which can change over time.
| # TODO-barret-q: Is this used? | ||
| self.api_key = api_key |
There was a problem hiding this comment.
I couldn't see where this is used. Just wanted to be sure I didn't lose this one as it is related to auth.
| # TODO-barret-future-q: Should this be destroy? | ||
| def delete(self) -> None: |
There was a problem hiding this comment.
Should this be .delete() or .destroy()?
| # TODO-barret-future-q: Should this be `.all()`? | ||
| @overload | ||
| def find( | ||
| self, | ||
| *, | ||
| all: Optional[bool] = ..., | ||
| ) -> List[Session]: ... | ||
|
|
||
| # TODO-barret-future-q: Should this be `.find_by()`? | ||
| @overload | ||
| def find(self, **kwargs) -> List[Session]: ... | ||
|
|
||
| def find(self, **kwargs) -> List[Session]: |
There was a problem hiding this comment.
No. all: bool is a query parameter. Instead, it should be **kwargs: Unpack[Sessions._FindAttrs]
schloerke
changed the title
|
Hey there! 👋 We noticed that the title of your pull request doesn't follow the Conventional Commits specification. To ensure consistency, we kindly ask you to adjust the title accordingly. Here are the details: |
|
After conversations with Taylor, we are not going to merge this PR. However, there are good ideas within it. Will keep the PR open until issues are made and/or components pulled out. |
Fixes #324
Goals:
.update()-like methods now return a new object..params: ResourceParameters(Rename to._ctx: ContextContextto extend it with more information rather than expanding parameters. By doing so, the names and usage are consistent. (Hard to distinguish which GUIDguidrepresents )ContextP[ContextT]class where possible to give._ctx: ContextTa type.ActiveDictclass to request the data for initialization. This reduces multiple duplications of logic to a single spotApiCallMixinwithin classes to reduce how urls are constructed and processed. Given a.ctx: Contextand._path: str, many calling methods can be handled under the hood.Note - Classes that do not follow the read-only approach:
EnvVarsContentItemVanityMixin