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

Preparation for 1.16.0 release #175

Merged
merged 2 commits into from
Oct 29, 2024
Merged

Preparation for 1.16.0 release #175

merged 2 commits into from
Oct 29, 2024

Conversation

denisenkom
Copy link
Owner

Fixes mypy failures

Fix for test_sec_context test

Copy link

codecov bot commented Oct 12, 2024

Codecov Report

Attention: Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.32%. Comparing base (18e6427) to head (835b699).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/pytds/tls.py 0.00% 4 Missing ⚠️
src/pytds/tds_base.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
- Coverage   89.35%   89.32%   -0.04%     
==========================================
  Files          44       44              
  Lines        8129     8130       +1     
==========================================
- Hits         7264     7262       -2     
- Misses        865      868       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@mjk4 mjk4 left a comment

Choose a reason for hiding this comment

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

Just some py 3.9 compatibility concerns.

@@ -955,7 +956,7 @@ def __init__(self) -> None:
self.validate_host = True
self.enc_login_only = False
self.enc_flag = 0
self.tls_ctx = None
self.tls_ctx: None | OpenSSL.SSL.Context = None
Copy link

Choose a reason for hiding this comment

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

allowing pipes for Union types doesn't work in python 3.9: https://peps.python.org/pep-0604/

Choose a reason for hiding this comment

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

Does this mean 1.16.0 will not work in python 3.9, or this just affects the package building?

Copy link

Choose a reason for hiding this comment

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

It won't work on python 3.9 at all. For example, this code works on 3.11 but is a syntax error on 3.9:

class Foo:
    asdf: None | int

Choose a reason for hiding this comment

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

I see. Thanks

Copy link
Owner Author

Choose a reason for hiding this comment

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

This module has

from __future__ import annotations

Which apparently enables Union type support for Python starting at least from 3.8

For example this code works on Python 3.8:

from __future__ import annotations
class Foo:
    asdf: None | int

host, _, _ = socket.gethostbyname_ex(server)
target_name = "MSSQLSvc/{0}:1433".format(host)
# getting current host's name
host, _, _ = socket.gethostbyname_ex("")
Copy link

Choose a reason for hiding this comment

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

IDK about other versions of python, but at least on 3.9 I get a Resolver Error when a blank str is given to socket.gethostbyname_ex.

Copy link

Choose a reason for hiding this comment

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

Actually, it doesn't seem to work with 3.11, either.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks for reviewing, will take a look.
Weirdly tests passed on Python 3.8 https://ci.appveyor.com/project/denisenkom/pytds/builds/50784050

Copy link
Owner Author

Choose a reason for hiding this comment

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

This works on Windows on Python 3.8 and 3.12 and this test is only executed on Windows.

Copy link

@mjk4 mjk4 left a comment

Choose a reason for hiding this comment

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

Please fix noted issues.

@Wedge009
Copy link

Not intending to add any pressure, just trying to get a sense of your schedule: when might we expect the next release? I'm interested on the assumption that it will resolve #167/#176.

Failing that, which release was the last one that didn't exhibit this issue?

@denisenkom denisenkom requested review from mjk4 and BaoshanGu October 27, 2024 02:09
Copy link

@mjk4 mjk4 left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

@BaoshanGu BaoshanGu left a comment

Choose a reason for hiding this comment

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

Thanks. We will keep an eye on the "|".

@denisenkom denisenkom merged commit a273130 into master Oct 29, 2024
2 of 4 checks passed
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.

4 participants