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

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

Merged
merged 1 commit into from
Feb 21, 2018

Conversation

nickpegg
Copy link
Contributor

@nickpegg nickpegg commented Feb 21, 2018

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

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, understand, and PRs easier to review.

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

Fixes dropbox#248
@nickpegg nickpegg requested a review from a team February 21, 2018 00:57
@narJH27
Copy link
Contributor

narJH27 commented Feb 21, 2018

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
Copy link
Contributor Author

@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
Copy link
Contributor

narJH27 commented Feb 21, 2018

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

@nickpegg
Copy link
Contributor Author

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.

@nickpegg nickpegg merged commit 3495329 into dropbox:develop Feb 21, 2018
@nickpegg nickpegg deleted the split-models branch February 21, 2018 21:49
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

Successfully merging this pull request may close these issues.

None yet

2 participants