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

Remove diffsync field from DiffSyncModel #256

Open
jamesharr opened this issue Nov 20, 2023 · 4 comments
Open

Remove diffsync field from DiffSyncModel #256

jamesharr opened this issue Nov 20, 2023 · 4 comments

Comments

@jamesharr
Copy link
Contributor

Environment

  • DiffSync version: 1.9.0

Proposed Functionality

  • Remove the diffsync field from DiffSyncModel
  • Add diffsync to DiffSyncModel.update and DiffSyncModel.create method signatures

There are a few reasons behind my thinking: 1) It makes the API a bit more consistent 2) It might make it easier for Python to run GC if there's no/fewer cyclical references between objects 3) I didn't see a lot of references to this field outside of create/update/delete

This is likely appropriate for diffsync v2 #232 since it breaks the API.

Other feature requests like #255 may benefit from this change since it would mean there is no assumption that DiffSyncModel has a reference to a given DIffSync

Use Case

class DiffSyncModel(BaseModel):
    @classmethod
    def create(cls, diffsync: "DiffSync", ids: Dict, attrs: Dict) -> Optional[Self]:
        # Method signature remains the same
        ...

    def update(self, diffsync: "DiffSync", attrs: Dict):
        ...

    def delete(self, diffsync: "DiffSync"):
        ...
jamesharr pushed a commit to jamesharr/diffsync that referenced this issue Nov 20, 2023
- Remove DiffSyncModel.diffsync field
- Add diffsync parameter to DiffSyncModel.create/update/delete

This closes networktocode#256
@cardoe
Copy link

cardoe commented Jan 2, 2024

This actually similarly relates to nautobot/nautobot-app-ssot#272 because of the job=self reference which is being passed in many of Nautobot's usage simply to pass a logger down to the model methods of create, update, delete.

What do you ultimately feel the need to pass the DiffSync down to each of the models? Since the DiffSync holds all the models which are having an operation performed on them?

@Kircheneer
Copy link
Collaborator

Yeah primarily for me its about having the logging context available.

@jamesharr
Copy link
Contributor Author

Yeah primarily for me its about having the logging context available.

I don't necessarily need the DiffSync or Logger in the model itself. I need the logger in the CRUD methods and it's also very helpful to have the DiffSync instance in the CRUD methods because that's where we wind up stashing some things like the Nautobot API client, NSO API client, DB connections

@cardoe
Copy link

cardoe commented Jan 9, 2024

But should you? Or should you have a generic AdapterConfig object that let's each adapter define the parameters and what gets sent to them. A very rough crappy example... cardoe@ffca6ef

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

No branches or pull requests

3 participants