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

fix: get_size should always return size as tuple of int #111

Merged
merged 4 commits into from
Sep 13, 2022

Conversation

fabien-michel
Copy link
Contributor

@fabien-michel fabien-michel commented Mar 25, 2022

self.picture.width (and height) are now float (since django-filer PR#1226)

Description

Since the following django-filer PR: django-cms/django-filer#1226
the FilerImageField width and height are now float.

We expect get_size to return always int.

Related resources

#108

Checklist

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

self.picture.width (and height) are now float (since PR#1226)
@fabien-michel fabien-michel changed the title fix: get_size should always return an int fix: get_size should always return size as tuple of int Mar 25, 2022
@vinitkumar
Copy link
Member

@fabien-michel What happens if someone updates the picture plugin and is still on the old version o filer? Will this break?

@fabien-michel
Copy link
Contributor Author

No, I don't see any case where it can break something. The old filer version behavior was to return int, so parsing int to int is ok.

@marksweb
Copy link
Sponsor Member

marksweb commented Sep 7, 2022

@fabien-michel are you able to resolve conflicts please? We've not got permissions to modify this PR

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #111 (42818c9) into master (de8a8cd) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #111   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          242       244    +2     
  Branches        45        45           
=========================================
+ Hits           242       244    +2     
Impacted Files Coverage Δ
djangocms_picture/models.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@fsbraun
Copy link
Sponsor Member

fsbraun commented Sep 8, 2022

I assume the tests would run through if you changed

call_command('makemigrations', **options)

to

 call_command('makemigrations', 'djangocms_picture', **options)

(The migration test creates migrations for django cms 3.9 which is not due to djangocms_picture. From my point of view it should be sufficient to test if this app has all migrations it needs.)

@fabien-michel
Copy link
Contributor Author

@fsbraun Thanks for the suggestions on test fail. But I still dont understand why I need to do changes on test_migrations.py since I didn't change anything related to the database.

@marksweb
Copy link
Sponsor Member

marksweb commented Sep 8, 2022

@fabien-michel the suggestion to make the change is because the test is failing due to external apps creating migration files. So making the change suggested by @fsbraun causes the test to only try to create migrations for this app.

@fsbraun fsbraun merged commit 84e63d2 into django-cms:master Sep 13, 2022
gruy pushed a commit to gruy/djangocms-picture that referenced this pull request Dec 2, 2023
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

4 participants