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

Move __version__ to __init__.py #243

Closed
wants to merge 4 commits into from

Conversation

pepoluan
Copy link
Collaborator

@pepoluan pepoluan commented Feb 5, 2021

What do these changes do?

Move the __version__ attribute from smtp.py to __init__.py

Benefits:

  • Semantically more correct -- version applies to all aiosmtpd package, not just smtp.py
  • Prevent breakage when installing using setuptools<46.4.0 (see this PR which resulted in this change for 46.4.0

Are there changes in behavior for the user?

Only if user actively searches for __version__ in the source code of smtp.py (via regex, startswith, or AST) instead of importing that attribute from smtp.py

User that do from aiosmtpd.smtp import __version__ will NOT be affected, though it will be better if the import is done from aiosmtpd instead.

Related issue number

Fixes #241 (partially? Not sure if encoding error in the output posted in that issue is related to this...)

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • tox testenvs have been executed in the following environments:
    • Windows 10 (via PyCharm tox runner): (ALL)
    • Windows 10 (via PSCore 7.1.1): (ALL)
    • Windows 10 (via Cygwin): qa,py36-{nocov,cov}
    • Ubuntu 18.04 on WSL 1.0: (ALL) + pypy3-{nocov,cov,diffcov}
    • FreeBSD 12.2 on VBox: (ALL) + pypy3-{nocov,cov,diffcov}
    • OpenSUSE Leap 15.2 on VBox: (ALL) + pypy3-{nocov,cov,diffcov}
  • Documentation reflects the changes
  • Add a news fragment into the NEWS.rst file

PR Note is undergoing editing; will undraft when editing done.

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
pepoluan Pandu E POLUAN
This should solve issue aio-libs#241, in which setuptools < 46.4.0 tries to
naively import smtp.py (due to version being specified as "attr:
aiosmtpd.smtp.__version__" and barfs when encountering "from public
import public"

Moving __version__ to __init__.py seems to be the wisest -- and most
semantically accurate -- decision. Not only can setuptools < 46.4.0
simply do a naive import of __init__.py (which contains no deps), but
this signifies the version number applies to the _whole_ aiosmtpd
package instead of just the smtp.py module.

This impacts the following files
* setup.cfg -- scan inside aiosmtpd instead of aiosmtpd.smtp
* smtp.py -- it now needs to import
* conf.py -- no longer scan smtp.py; import from __init__ instead

Verified

This commit was signed with the committer’s verified signature. The key has expired.
pepoluan Pandu E POLUAN

Verified

This commit was signed with the committer’s verified signature. The key has expired.
pepoluan Pandu E POLUAN
In this case, ensure __version__ from __init__.py is exact same object
as __version__ from smtp.py

Verified

This commit was signed with the committer’s verified signature. The key has expired.
pepoluan Pandu E POLUAN
Copy link
Collaborator Author

@pepoluan pepoluan left a comment

Choose a reason for hiding this comment

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

No comments. Changes are simple and does not affect functionality.

@pepoluan pepoluan added this to the 1.3 milestone Feb 5, 2021
@pepoluan pepoluan marked this pull request as ready for review February 5, 2021 17:38
@pepoluan pepoluan requested a review from waynew February 5, 2021 17:38
@pepoluan
Copy link
Collaborator Author

pepoluan commented Feb 5, 2021

Since this is something simple, I'll just marinate for 1x24

Copy link
Collaborator

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Seems legit 👍

@pepoluan
Copy link
Collaborator Author

pepoluan commented Feb 6, 2021

After some thinking, I think I'll merge this with #242 instead. Because it was @manfred-kaiser who brought this to our attention.

@pepoluan pepoluan closed this Feb 6, 2021
@pepoluan pepoluan deleted the version-in-init branch February 7, 2021 08:52
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.

Can't install Version 1.2.4 - No module named 'public'
2 participants