javrasya / django-river

Django workflow library that supports on the fly changes ⛵
BSD 3-Clause "New" or "Revised" License
741 stars 105 forks source link

⬆️ Support Django 3.1 #180

Closed simkimsia closed 3 years ago

simkimsia commented 3 years ago

ref #179

simkimsia commented 3 years ago

@javrasya Let me know if I'm doing this correctly.

Once I get this working and accepted, I will make two other PRs:

  1. Add python 3.7
  2. Add python 3.8

I think you set python 3.6 as your default. I'm okay with that just that I want to make sure river is still compatible with 3.7 and 3.8.

The reason is because I hope to close #179

coveralls commented 3 years ago

Coverage Status

Coverage decreased (-81.6%) to 0.0% when pulling e292d9a9fcbbc34bd8ba6d08e0f5d2932b36076b on simkimsia:upgrade-django31 into 57fdcc1926adba86ef8697adb00e6a235a7bdbff on javrasya:master.

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 81.646% when pulling 9c0e4911ab5c93a810bbe07b582a144e9c73ead5 on simkimsia:upgrade-django31 into 57fdcc1926adba86ef8697adb00e6a235a7bdbff on javrasya:master.

simkimsia commented 3 years ago

Coverage Status

Coverage decreased (-81.6%) to 0.0% when pulling e292d9a on simkimsia:upgrade-django31 into 57fdcc1 on javrasya:master.

Sorry @javrasya I'm not sure why the coveraged dropped to zero. Can help me decipher this?

javrasya commented 3 years ago

Coverage Status Coverage decreased (-81.6%) to 0.0% when pulling e292d9a on simkimsia:upgrade-django31 into 57fdcc1 on javrasya:master.

Sorry @javrasya I'm not sure why the coveraged dropped to zero. Can help me decipher this?

I don't think it is a big deal. Don't worry about that. Few tests are broken because some error messages on the Django side with the newer version are different compared to the older versions.

simkimsia commented 3 years ago

I have looked. I have a situation where a test passes at 3.0 but fails at 3.1 django

I looked at how Django tests for the assertion message and apparently has teh same setup regardless if it's 3.0 https://github.com/django/django/blob/stable/3.0.x/tests/delete/tests.py#L70 or 3.1 https://github.com/django/django/blob/stable/3.1.x/tests/delete/tests.py#L72

so how do I solve this?

javrasya commented 3 years ago

I have looked. I have a situation where a test passes at 3.0 but fails at 3.1 django

I looked at how Django tests for the assertion message and apparently has teh same setup regardless if it's 3.0 https://github.com/django/django/blob/stable/3.0.x/tests/delete/tests.py#L70 or 3.1 https://github.com/django/django/blob/stable/3.1.x/tests/delete/tests.py#L72

so how do I solve this?

The error message in django v3.1 for delete protection violation is slightly different;

3.0: Cannot delete some instances of model 'Workflow' because they are referenced through <a> protected foreign key 3.1: Cannot delete some instances of model 'Workflow' because they are referenced through protected foreign key<s>

So the message pattern in the tests must be compatible with the change.

https://github.com/javrasya/django-river/blob/master/river/tests/models/test__transition_approval.py#L30 https://github.com/javrasya/django-river/blob/master/river/tests/models/test__transition_approval.py#L48 https://github.com/javrasya/django-river/blob/master/river/tests/models/test__transition_approval.py#L66

According to pychamcrest, the message we pass down to the expectation is a regexp pattern. So we can simply pass the following instead of full message;

        assert_that(
            calling(flow.workflow.delete),
            raises(ProtectedError, "Cannot delete some instances of model 'Workflow' .*")
        )
simkimsia commented 3 years ago

@javrasya thank you

What's your linter by the way?

Mine is black so it changes quite a bit of the formatting

image

javrasya commented 3 years ago

@javrasya thank you

What's your linter by the way?

Mine is black so it changes quite a bit of the formatting

image

That is a good question actually. I use PyCharm and my formatter is the one that comes with it out of the box except with a custom line limiter at 160 instead of 120;

Screen Shot 2021-01-02 at 15 43 53
simkimsia commented 3 years ago

So will u have any issues that I maintain my current linter?

javrasya commented 3 years ago

So will u have any issues that I maintain my current linter?

The only thing I noticed which I would like you to have is the line limit to be 160. Apart from that, it all looks good.

simkimsia commented 3 years ago

So will u have any issues that I maintain my current linter?

The only thing I noticed which I would like you to have is the line limit to be 160. Apart from that, it all looks good.

I have already done that

image

But no change for the code apparently

simkimsia commented 3 years ago

oh wait there's a change. I apologize. I will make a force-push to ensure it follows your 160 limit

simkimsia commented 3 years ago

Thank you @javrasya please review this PR and let me know if there's any other changes I need to make to close #179

javrasya commented 3 years ago

Thank you @javrasya please review this PR and let me know if there's any other changes I need to make to close #179

Amazing, I did small touches to make the diff look cleaner in this PR. Let's wait for one final build to pass and then it is time to merge this :-)

javrasya commented 3 years ago

Well done @simkimsia