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

remove six dependency #324

Merged
merged 7 commits into from
Oct 2, 2023
Merged

remove six dependency #324

merged 7 commits into from
Oct 2, 2023

Conversation

a-detiste
Copy link
Contributor

this is a lefotver of Python2 compatibility

@cdent
Copy link
Owner

cdent commented Sep 25, 2023

Thanks for this, will give it a proper look some time a bit later in the week.

Out of curiosity what prompted you to provide the pull request?

@a-detiste
Copy link
Contributor Author

Hi,

I'm reviewing random packages that still depends on six (/ needs a hand).

https://wiki.debian.org/Python3-six-removal

I provided once a -1400/+200 PR to a project,
I learn new things & have some fun.

Copy link
Collaborator

@FND FND left a comment

Choose a reason for hiding this comment

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

That's a commendable effort!

This all seems sensible upon cursory inspection - though I'm insufficiently contextualized to be a reliable reviewer. I've only noticed a few minor details, plus tests balk at a few stylistic issues (PEP 8).

gabbi/case.py Outdated Show resolved Hide resolved
gabbi/case.py Outdated Show resolved Hide resolved
gabbi/tests/test_fixtures.py Outdated Show resolved Hide resolved
Copy link
Owner

@cdent cdent left a comment

Choose a reason for hiding this comment

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

Thanks again for doing this, and following up with some fixes. There are a few issues remaining to get the pep8 test to pass.

Locally you can run tox -epep8 to check for the issues instead of waiting for CI (which clearly I need to adjust permissions on somehow).

If you haven't got time to make the changes let me know and I can do them.

gabbi/fixture.py Outdated Show resolved Hide resolved
gabbi/tests/__init__.py Show resolved Hide resolved
@a-detiste
Copy link
Contributor Author

I find the way the import are sorted completely unnatural.

But now... we have an actual problem to fix(ture) !

@cdent
Copy link
Owner

cdent commented Oct 2, 2023

But now... we have an actual problem to fix(ture) !

Looks like the fix for that is this:

diff --git a/gabbi/fixture.py b/gabbi/fixture.py
index 4538dee..03b8e10 100644
--- a/gabbi/fixture.py
+++ b/gabbi/fixture.py
@@ -95,4 +95,4 @@ def nest(fixtures):
             except Exception:
                 exc = sys.exc_info()
         if exc != (None, None, None):
-            raise exc[0]
+            raise exc[0](exc[1]).with_traceback(exc[2])
diff --git a/gabbi/tests/test_suite.py b/gabbi/tests/test_suite.py
index d550a4a..e31b0c5 100644
--- a/gabbi/tests/test_suite.py
+++ b/gabbi/tests/test_suite.py
@@ -19,6 +19,7 @@ import unittest
 from gabbi import fixture
 from gabbi import suitemaker

+FIXTURE_METHOD = 'start_fixture'
 VALUE_ERROR = 'value error sentinel'


@@ -54,3 +55,4 @@ class SuiteTest(unittest.TestCase):

         self.assertIn('foo_alpha', str(errored_test))
         self.assertIn(VALUE_ERROR, trace)
+        self.assertIn(FIXTURE_METHOD, trace)

If you're happy with that, please apply that and then I'll merge this and make a new release and we can call this a job very well done.

(Note I haven't checked pep8 on these changes yet...)

(Again, if you don't have the time, let me know and I can do it).

@a-detiste a-detiste requested a review from cdent October 2, 2023 19:58
Copy link
Owner

@cdent cdent left a comment

Choose a reason for hiding this comment

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

Nice, your solution is a bit simpler than mine, but if you could also add the suggested test change that I made in my proposed diff that would be great. It makes sure that the traceback points to the right location.

gabbi/fixture.py Show resolved Hide resolved
@@ -54,3 +55,4 @@ def test_suite_catches_fixture_fail(self):

self.assertIn('foo_alpha', str(errored_test))
self.assertIn(VALUE_ERROR, trace)
self.assertIn(FIXTURE_METHOD, trace)
Copy link
Owner

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got the unicode missing rectangle symbol. I had to google it :-) ([👍 Pouce Vers Le Haut sur Apple iOS 11.1])

@cdent cdent merged commit de6554d into cdent:main Oct 2, 2023
14 checks passed
@cdent
Copy link
Owner

cdent commented Oct 2, 2023

Thanks again!

2.10.0 has been released.

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