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

Update for fabric3.0 #46

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Update for fabric3.0 #46

wants to merge 8 commits into from

Conversation

LecrisUT
Copy link

@LecrisUT LecrisUT commented Mar 15, 2023

  • Resolve issues for fabric 3.0
  • Pep621 modernization
  • Changed .format to fstrings for more readability

Fixes: #41
Depends-on: #47

@LecrisUT LecrisUT marked this pull request as draft March 15, 2023 13:41
@bpluly
Copy link

bpluly commented Mar 24, 2023

Just as commentary, patching with this I was able to install the library and everything seems fine, I don't have all the test tools and haven't addressed that. I did wonder why setup.py was removed as it should still install with a toml file. Unless this is part of the work in progress it's worth calling it out in the PR.

@LecrisUT
Copy link
Author

The change from setup.py to pyproject.toml is just implementing PEP621 since most projects are/should be migrating to it. That part is finished. For this PR, the only reason it is in draft is because I don't feel confident without getting the pytest running. The test method implemented also seems to contradict the recommended approach using MockContext. If you have any hints on how to get the tests I can translate them over.

@LecrisUT LecrisUT marked this pull request as ready for review March 24, 2023 16:07
@LecrisUT
Copy link
Author

LecrisUT commented Mar 24, 2023

Ok, the tests are running again. I was missing a dependency. I'll try to see if I can fix the test failures. Failed docs are related to docstring, got no idea how to fix those.

@bitprophet bitprophet mentioned this pull request Mar 24, 2023
@bitprophet
Copy link
Member

Thanks for poking at this. I don't have a lot of time to dig right now but hopefully sometime soon-ish.

I did notice you set the dep to fabric>=3 - the backwards incompat changes in 3.x were quite minor (the big one was just dropping Py<3.6 support, which is handle-able by users' pips) so it's not clear to me what exactly broke here under 3.x. I think it's more likely the breakage was on 2.x (which was a total rewrite) and you should be able to set the pyproject field to >=2 instead?

@bitprophet bitprophet added this to the Next big release milestone Mar 24, 2023
Copy link
Author

@LecrisUT LecrisUT left a comment

Choose a reason for hiding this comment

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

@bitprophet I've highlighted the main changes. Others were stylistic changes. And indeed, at least from the pytest, there didn't seem to be anything drastically changed, the only one seems to be with parsing the docstring, if you can assign someone to take a look at that, that would be wonderful

@@ -1,6 +1,6 @@
from importlib import import_module

from invocations import docs, travis
Copy link
Author

Choose a reason for hiding this comment

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

invocations.travis was dropped.

src/patchwork/util.py Outdated Show resolved Hide resolved
@@ -2,8 +2,6 @@
File transfer functionality above and beyond basic ``put``/``get``.
"""

from invoke.vendor import six
Copy link
Author

Choose a reason for hiding this comment

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

invoke does not bundle six, moved to str

pyproject.toml Outdated Show resolved Hide resolved
Signed-off-by: Cristian Le <[email protected]>
@LecrisUT LecrisUT force-pushed the fabric3 branch 3 times, most recently from 2800b11 to 23c9aba Compare April 19, 2023 08:08
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
@LecrisUT LecrisUT force-pushed the fabric3 branch 2 times, most recently from 0920324 to 8cfc5c3 Compare April 19, 2023 08:36
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
@LecrisUT
Copy link
Author

@bitprophet I hope the CIs here help. Please check the runs on my fork until the github action is enabled.

Are you ok with updating the minimum python requirement to 3.7? 3.6 does not support PEP621

@JoshYuJump
Copy link

@bitprophet I hope the CIs here help. Please check the runs on my fork until the github action is enabled.

Are you ok with updating the minimum python requirement to 3.7? 3.6 does not support PEP621

Python 3.7 has reached EOF, may by minimum python requirement should be 3.8

@SamuelMarks
Copy link

I'm supporting 2.7, 3.5, 3.6, 3.7, 3.8, 3.9, 3.10, 3.11, 3.12 with Fabric and Patchwork. Would be great if this could be merged without breaking support for 3.5+ (so I can easily remain on Fabric 2.7.1 on python_version<'3.5' and newer versions later)

@LecrisUT
Copy link
Author

LecrisUT commented Jan 2, 2024

@SamuelMarks If you want, you can run these tests on your fork with the minimum Python version you wish, and report back the patches needed to make it compatible. I don't remember if I checked the minimum requirements below those versions

@SamuelMarks
Copy link

@LecrisUT Only just got the chance. It should now support Python 2.7 (added the older setup.py which is compatible with older versions)

@geoffrey-eisenbarth
Copy link

@bitprophet Any movement on this? Thanks :)

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.

cannot import name 'six' from 'invoke.vendor'
6 participants