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

Better Plone 4 / Python 2 compatibility #233

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Better Plone 4 / Python 2 compatibility #233

merged 4 commits into from
Jan 10, 2024

Conversation

thet
Copy link
Member

@thet thet commented Jan 5, 2024

When I‌ tried to install this package in Plone 4 it took me some time to get the versions straight. It would have saved me some time if I had read the Plone 4 install section in the README, but I didn't and it is well hidden.

I think having an easy installation experience in Plone 4 with Python 2 is quite important for this package as this is probably a frequent use case.

This PR should fix that out of the box and improve the README a bit, as of the time of this writing. There is a good chance that it will break again, however, for now it does work fine.

/cc @canepan

thet added 3 commits January 5, 2024 19:50
The original author information should be at the end and not somewhere
in between.
@thet thet requested review from macagua, fredvd and pbauer January 5, 2024 18:56
install_requires.append("plone.restapi < 8")
install_requires.append("plone.schema < 2")
install_requires.append("PyJWT < 2")
install_requires.append("pyrsistent < 0.16.0")
Copy link
Member

Choose a reason for hiding this comment

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

It looks to me like you're adding pins for Plone 4, but doing it based on the presence of Python 2. It is also possible to use Python 2 with Plone 5.1 and 5.2

Copy link
Member Author

Choose a reason for hiding this comment

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

I‌ tested only in Plone 4.
But above these thresholds there is only Python 3 support - also for plone.schema, plone.restapi and plone.api.
I think this should be safe.

Copy link
Member

Choose a reason for hiding this comment

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

@thet I think you are right.

@thet
Copy link
Member Author

thet commented Jan 10, 2024

I‌ take that as a go :D
Time to merge, I've tested in in Plone 4 and Plone 6 and it works just fine.

@thet thet merged commit f96a3bd into main Jan 10, 2024
11 checks passed
@thet thet deleted the plone4 branch January 10, 2024 23:34
@pbauer
Copy link
Member

pbauer commented Jan 12, 2024

@thet weird, since this merge this packages main page does not render anymore in github, same for forks.
Also maybe related: @flipmcf ran into this issue during installation:

mr.developer: Updated 'collective.exportimport' with git.
Exception in thread Thread-2:
Traceback (most recent call last):
  File "/usr/lib/python3.9/threading.py", line 954, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.9/threading.py", line 892, in run
    self._target(*self._args, **self._kwargs)
  File "/home/mcfaddenm/repos/plone5.2_clean/rfasite/rfasite/eggs/mr.developer-2.0.1-py3.9.egg/mr/developer/common.py", line 181, in worker
    for lvl, msg in wc._output:
ValueError: too many values to unpack (expected 2)

@pbauer
Copy link
Member

pbauer commented Jan 13, 2024

The issue has gone away :)

@thet
Copy link
Member Author

thet commented Jan 13, 2024

Wow, great :)
Both issues?

@flipmcf issue looks like if you got a Python 2 incompatible package, actually the kind of problem my PRs here should have fixed.
But there will be similar problem popping up as soon as another dependency is released as Python 3 only package where we do not have any requirement constraints.

The other issue - I updated the README - maybe I created an .rst error.

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.

3 participants