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

Deleting import zope.interface implements #1189

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Conversation

nilbacardit26
Copy link
Contributor

@nilbacardit26 nilbacardit26 commented Nov 16, 2023

Every time I install guillotina as a dependency I encounter this error:
Error importing plugin "guillotina.tests.fixtures": cannot import name 'implements' from 'zope.interface. implements is not there anymore in version 6.1 'zope.interface

I do not know what this line does, implements is not used anywhere. I passed all the test without this line. Some of you know why is it there?
https://github.com/plone/guillotina/blob/master/guillotina/component/interfaces.py#L19

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2023

Codecov Report

Merging #1189 (ef0ae70) into master (29a6958) will decrease coverage by 0.0%.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1189     +/-   ##
========================================
- Coverage    94.6%   94.6%   -0.0%     
========================================
  Files         377     377             
  Lines       32771   32770      -1     
========================================
- Hits        30992   30991      -1     
  Misses       1779    1779             
Files Coverage Δ
guillotina/component/interfaces.py 100.0% <ø> (ø)

@vangheem
Copy link
Member

It's a vanity import to be able to use the implements interface. If that is no longer used/support, remove the import!

I don't think it's a good idea to hard pin a version -- it can be annoying for consumers

@nilbacardit26
Copy link
Contributor Author

@vangheem done, I will run the tests I have in another project just to be sure this import is not breaking anything

@vangheem
Copy link
Member

@nilbacardit26 everything look good?

We can merge then

@nilbacardit26
Copy link
Contributor Author

@vangheem yes, I am not authorized to merge, feel free to do it

@nilbacardit26 nilbacardit26 changed the title pinning zope.interface in setup to 5.0.1 Deleting import zope.interface implements Nov 20, 2023
@bloodbare bloodbare merged commit e0f1283 into master Nov 20, 2023
22 checks passed
@bloodbare bloodbare deleted the pinning-implements branch November 20, 2023 10:30
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.

5 participants