search

dropbox / nsot

Network Source of Truth is an open source IPAM and network inventory database
https://nsot.readthedocs.io
Other
399 stars 66 forks source link

Split nsot/models.py to one model per file #317

Closed nickpegg closed 6 years ago

nickpegg commented 6 years ago

This breaks up the models.py monolith so that each model (and its associated other bits) each live in their own file. Hopefully this makes things easier to read, easier to understand, and PRs easier to review.

This also preserves the current import behavior, meaning something like from nsot.models import Device still works as expected.

One downside is that we lose git history on each of these files, but this commit message hopefully serves as a good redirection to find the original file, and I think the benefits outweigh that negative.

Fixes #248

narJH27 commented 6 years ago

This looks great! Just one suggestion from my end, there are too many repetitions of clean_site(), clean_fields() and save() which I feel could easily be moved to a base class from where the specific model classes could subclass and overload the methods if needed. This is just to avoid duplication of the same block of code over and over again.

nickpegg commented 6 years ago

@narJH27 my intention was to leave the actual code alone as much as possible to reduce the risk of this change. I simply copied the classes from models.py to their respective files.

I guess it depends on where this change should land: is it destined for 1.x and the develop branch, or should this change go into v2.0? If it's the former, I want to stay low-risk and change as little as possible. If instead this is meant for v2.0, then we can totally go to town refactoring these models!

narJH27 commented 6 years ago

I agree with your approach. I'd defer to @jathanism for his inputs on where we should land this change.

nickpegg commented 6 years ago

Summary from an off-GitHub conversation: Any refactoring that does changes to the code outside of just copying the code to new files is out of scope for this PR, and a new PR should be raised for that.