networktocode
Remove `diffsync` field from DiffSyncModel
Environment
- DiffSync version: 1.9.0
Proposed Functionality
- Remove the
diffsyncfield from DiffSyncModel - Add
diffsyncto 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"):
...
This actually similarly relates to https://github.com/nautobot/nautobot-app-ssot/issues/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?
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
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... https://github.com/cardoe/diffsync/commit/ffca6ef9fb3af877178d37678c77d57f93fb4321
So I think this was opened due to a lack of knowledge in part. First, the field has been changed to adapter and it has always been passed to update() off the self attribute along with delete(). The Job itself is also off of adapter so you can find the logger there. You also have direct access to any Job vars from there too. I'm going to close this for now unless there's something I'm missing.
