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

[#480] Remove Python2 compatibility from code base #640

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

d-w-moore
Copy link
Collaborator

With this change, we remove all code meant explicitly to deal with the possibility of the PRC's running under Python 2. Especially the awkward (and sometimes voluminous) parts that interfere with a ready understanding of the code base as a whole.

@d-w-moore d-w-moore marked this pull request as draft October 15, 2024 20:24
@trel
Copy link
Member

trel commented Oct 16, 2024

looking good so far. please assert here how testing goes.

irods/message/__init__.py Outdated Show resolved Hide resolved
@d-w-moore d-w-moore force-pushed the 480.m branch 3 times, most recently from 41b4824 to 6b0d13d Compare October 16, 2024 14:33
@d-w-moore
Copy link
Collaborator Author

looking good so far. please assert here how testing goes.

have already done some piecemeal testing with good results. will run a full test later.

@korydraughn
Copy link
Contributor

Excellent! Keep pushing.

@d-w-moore d-w-moore force-pushed the 480.m branch 2 times, most recently from d478f99 to fac1329 Compare October 17, 2024 14:12
@d-w-moore d-w-moore marked this pull request as ready for review October 17, 2024 15:24
@d-w-moore
Copy link
Collaborator Author

There may still be a few points I haven't thought of, but presently the code is exclusively Python3-compatible (python2 -c"import irods" fails !!) and passes all tests in the suite.

@d-w-moore
Copy link
Collaborator Author

.... now with minimal squashing

@korydraughn
Copy link
Contributor

Amazing work!

We'll start the review process.

Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Let's get another set of eyes on this before squashing/merging.

Good stuff!

irods/test/data_obj_test.py Show resolved Hide resolved
@korydraughn
Copy link
Contributor

There may still be a few points I haven't thought of, but presently the code is exclusively Python3-compatible (python2 -c"import irods" fails !!) and passes all tests in the suite.

How much does python2 -c"import irods" actually catch? Do you know?

I'm just curious.

@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Oct 17, 2024

There may still be a few points I haven't thought of, but presently the code is exclusively Python3-compatible (python2 -c"import irods" fails !!) and passes all tests in the suite.

How much does python2 -c"import irods" actually catch? Do you know?

I'm just curious.

Here's what happens if you try importing the PRC at tip of this branch currently:

Python 2.7.18 (default, Jan 31 2024, 16:23:13) 
[GCC 9.4.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import irods
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/daniel/ppy3/prc/irods/__init__.py", line 85, in <module>
    from . import client_configuration
  File "/home/daniel/ppy3/prc/irods/client_configuration/__init__.py", line 40
    class ConnectionsProperties(iRODSConfiguration, metaclass = iRODSConfigAliasMetaclass):
                                                              ^
SyntaxError: invalid syntax
>>> irods
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'irods' is not defined
>>> 

As a result of the errors evoked, not the irods namespace, nor module, nor any submodules will be imported (as you might well intuit by that last command). Which means all code in the PRC is unavailable to the interpreter under Python2

@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Oct 17, 2024

Amazing work!

We'll start the review process.

Great!...
Btw, I didn't mean to put the" laugh" emoji there. Joy is more what I wanted to express.

@trel
Copy link
Member

trel commented Oct 18, 2024

Can we... be kinder to anyone who might stumble into this by accident...

like...

import sys

if sys.version_info[0] < 3:
    raise RuntimeError("This library is only supported on Python 3 and above.")

But also check the minor version while we're there...

@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Oct 18, 2024

Can we... be kinder to anyone who might stumble into this by accident...

like...

import sys

if sys.version_info[0] < 3:
    raise RuntimeError("This library is only supported on Python 3 and above.")

But also check the minor version while we're there...

Good point. We'll put that into irods/__init__.py . It will work fine even if sub-namespaces are (naively) preferentially imported, because in order to import those, Python needs to import the parent modules first. (In other words, import irods.manager.data_object_manager has to import irods as the first step.

@trel
Copy link
Member

trel commented Oct 18, 2024

nice and clean.

irods/__init__.py Outdated Show resolved Hide resolved
Copy link

@FifthPotato FifthPotato left a comment

Choose a reason for hiding this comment

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

Seems pretty complete to me. Couldn't find anything wrong, so I think it looks good!

setup.py Outdated Show resolved Hide resolved
@trel
Copy link
Member

trel commented Oct 22, 2024

codacy has some opinions... but then... are we ready to squash?

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

I second a squash

@alanking
Copy link
Contributor

Created #644 for f-strings in logging statements.

Created #645 for naked asserts.

Created #646 for import ... as issue.

"Possibly unused variable" in a test seems... not worth the review/test time, but I say we address that in this PR if we address it at all (irods/test/collection_test.py line 171).

@trel
Copy link
Member

trel commented Oct 22, 2024

please grep for six and remove all instances found for this PR.

@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Oct 22, 2024

please grep for six and remove all instances found for this PR.

Done. All gone!

$ grep -rwI six|grep -v '\.git/'
irods/test/message_test.py:        ivp.svalue = ['five', 'six']
irods/test/message_test.py:        expected = "<GenQueryInp_PI><maxRows>4</maxRows><continueInx>3</continueInx><partialStartIndex>2</partialStartIndex><options>1</options><KeyValPair_PI><ssLen>2</ssLen><keyWord>one</keyWord><keyWord>two</keyWord><svalue>three</svalue><svalue>four</svalue></KeyValPair_PI><InxIvalPair_PI><iiLen>2</iiLen><inx>4</inx><inx>5</inx><ivalue>1</ivalue><ivalue>2</ivalue></InxIvalPair_PI><InxValPair_PI><isLen>2</isLen><inx>1</inx><inx>2</inx><svalue>five</svalue><svalue>six</svalue></InxValPair_PI></GenQueryInp_PI>"
irods/test/message_test.py:        self.assertEqual(gq2.InxValPair_PI.svalue, ["five", "six"])

@trel
Copy link
Member

trel commented Oct 22, 2024

alright - squash this. let's get it in.

@d-w-moore
Copy link
Collaborator Author

"Possibly unused variable" in a test seems... not worth the review/test time, but I say we address that in this PR if we address it at all (irods/test/collection_test.py line 171).

Is used in a format string.

@d-w-moore
Copy link
Collaborator Author

Tests pass , awaiting approval

@alanking
Copy link
Contributor

Yes, please squash.

@d-w-moore
Copy link
Collaborator Author

alright - squash this. let's get it in.

ok - sorry didn't see this til now

@d-w-moore
Copy link
Collaborator Author

squash done

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

You said tests are passing, everything checks out... I say # it.

@d-w-moore
Copy link
Collaborator Author

#'d

@trel
Copy link
Member

trel commented Oct 22, 2024

a note that we've also marked this as ready for Python 3.13.

@alanking alanking merged commit 5c1ba1e into irods:main Oct 22, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants