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

Add support for adding on_delete for ForeignKey and OneToOneField #117

Merged
merged 9 commits into from
Jul 4, 2020

Conversation

cvanderkolk
Copy link
Contributor

@cvanderkolk cvanderkolk commented Jun 29, 2020

Adding a feature to handle adding the on_delete argument to ForeignKey and OneToOneField types: https://docs.djangoproject.com/en/dev/releases/1.9/#foreignkey-and-onetoonefield-on-delete-argument

TODO:

  • Add some more tests
  • Address adding the models import if it doesn't exist
  • Support detecting the on_delete call when passed as the second position argument
  • Fix on_delete matcher which doesn't seem to be finding things right
  • Test internally with a complicated codebase 💃

Stretch:

  • Add an input so folks can add in subclasses (Edit: Probably going to do this in a future PR)

Refs #23

@browniebroke
Copy link
Owner

Thanks for the PR, this will be a very nice one to add.

The test failed because I didn't setup codecov correctly, apparently GitHub secrets aren't available to forks, which now that I think about it, makes sense. I've fixed it in #118, you might want to update your fork to get this change.

Let me know if you have any questions.

Thanks again!

@cvanderkolk cvanderkolk marked this pull request as ready for review July 1, 2020 15:58
@cvanderkolk
Copy link
Contributor Author

@browniebroke thanks a bunch for taking a look at that test—I merged in those changes and things are working as expected again! 👍 This is looking pretty good! I've added some unit tests and put it thru its paces with some projects I have running locally.

I have an instance where I'd like to use this for a field that subclasses OneToOneField, do you think it's worth adding functionality via CLI to define a path to a subclass field so we can handle those too?

@browniebroke browniebroke added the enhancement New feature or request label Jul 1, 2020
Copy link
Owner

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! Thank you for this awesome contribution.

Looking at the test, it looks like you're testing mostly for fixing migration files. Can we add a couple for fixing model definitions as well? For example:

class MyModel(models.Model):
	user = models.ForeignKey('auth.User')

I'll look at your question and see if I have a better suggestion

tests/visitors/test_models.py Outdated Show resolved Hide resolved
tests/visitors/test_models.py Outdated Show resolved Hide resolved
tests/visitors/test_models.py Outdated Show resolved Hide resolved
django_codemod/visitors/models.py Outdated Show resolved Hide resolved
Copy link
Owner

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are 2 minor nitpicking that Black doesn't handle properly yet.

django_codemod/visitors/models.py Outdated Show resolved Hide resolved
django_codemod/visitors/models.py Outdated Show resolved Hide resolved
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jul 1, 2020

Sourcery Code Quality Report (beta)

✅ Merging this PR will increase code quality in the affected files by 0.08 out of 10.

Before After Change
Quality 8.96 9.05 0.08
Complexity 1.13 1.0 -0.13
Method Length 38.84 35.59 -3.25
Lines 471 698 227
Changed files Quality Before Quality After Quality Change
django_codemod/visitors/init.py 8.57 8.53 -0.04 ⬇️
django_codemod/visitors/models.py 8.2 8.42 0.22 ⬆️
tests/test_cli.py 9.22 9.21 -0.01 ⬇️
tests/visitors/test_models.py 9.4 9.5 0.1 ⬆️

Here are some functions in these files that still need a tune-up:

File Function Quality Recommendation
django_codemod/visitors/models.py ModelsPermalinkTransformer.leave_ImportFrom 4.45 Split out functionality

Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Let us know what you think of it via email or our Gitter!

@cvanderkolk
Copy link
Contributor Author

@browniebroke thanks for the feedback! I just submitted some changes based on your suggestions.

@browniebroke browniebroke changed the title Add support for adding on_delete for ForeignKey and OneToOneFields Add support for adding on_delete for ForeignKey and OneToOneField Jul 4, 2020
@browniebroke browniebroke merged commit 008ac2a into browniebroke:master Jul 4, 2020
@browniebroke
Copy link
Owner

Thanks for this awesome contribution! This is a big one, wish I had it back when I upgraded 😄

@browniebroke
Copy link
Owner

@all-contributors please add @cvanderkolk for code

@allcontributors
Copy link
Contributor

@browniebroke

I've put up a pull request to add @cvanderkolk! 🎉

@browniebroke browniebroke added this to the Django 2.0 milestone Jul 4, 2020
@cvanderkolk cvanderkolk deleted the feature/add-on-delete branch July 6, 2020 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants