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

Using a ssl context object to avoid deprecation warnings in py3 #451

Merged
merged 1 commit into from Sep 14, 2022

Conversation

Yomguithereal
Copy link
Contributor

No description provided.

@hugovk
Copy link
Member

hugovk commented Sep 14, 2022

Please could you add comments to say which bit is for Python 2 and which for 3?

That will make it easier to clean up the code when the time comes.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3052768982

  • 5 of 12 (41.67%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 31.37%

Changes Missing Coverage Covered Lines Changed/Added Lines %
twitter/api.py 5 6 83.33%
twitter/stream.py 0 6 0.0%
Totals Coverage Status
Change from base Build 3051803496: 0.05%
Covered Lines: 655
Relevant Lines: 2088

💛 - Coveralls

@Yomguithereal
Copy link
Contributor Author

Please could you add comments to say which bit is for Python 2 and which for 3?

The code I wrote works the same in py2 and py3 as far as I know so nothing to clean up when the time comes (to kill py2 I suppose?). It's just that the earlier PR was triggering deprecation warning on py >= 3.6. Now it does not.

@boogheta
Copy link
Collaborator

boogheta commented Sep 14, 2022

I'm the one who sollicitated the PR to @Yomguithereal following the previous one.
This solves an issue we met while using the lib on recent Mac where python could not find on its own where the certifs resides on the OS.
This makes me wonder actually whether we should add to the matrix some tests under macos/windows as well, although that's already a lot of tweets sent whenever tests are run so not sure. What do you think @hugovk?

@boogheta boogheta merged commit fbc7aa5 into python-twitter-tools:master Sep 14, 2022
@hugovk
Copy link
Member

hugovk commented Sep 14, 2022

I think it's a good idea to test at least one on Windows and macOS. We don't necessarily need to add them to the full matrix to get the benefit.

We currently have:

            "2.7",
            "3.6",
            "3.7",
            "3.8",
            "3.9",
            "3.10",
            "3.11-dev",
            "pypy2.7",

We could switch out one of these middle versions to run on Windows instead of Ubuntu, and another to run on macOS.

In fact, we could remove some of those middle Ubuntu ones anyway, some projects only test on lower and upper bounds. So for example:

            "2.7",
            "3.6",
-           "3.7",
-           "3.8",
-           "3.9",
            "3.10",
            "3.11-dev",
            "pypy2.7",

Also Python 3.6 has been EOL since last year (2021-12-23) so that could be dropped. (Plus I still recommend EOL 2.7 being dropped (2020-01-01) ;)

@boogheta
Copy link
Collaborator

boogheta commented Sep 14, 2022

I like the idea to test some versions with mac and windows, for instance we could remove 3.7, and run 3.8 under windows and 3.9 under macOS ?
I know for 2.7, but I kinda like that the code is robust enough that it still supports it so far and I would be in favor of keeping it unless it becomes a maintenance charge, but that's only my opinion :)

@hugovk
Copy link
Member

hugovk commented Sep 14, 2022

Sounds good, I'll create a PR. Fine with me if you'd like to keep 2.7 longer. How about 3.6?

@boogheta
Copy link
Collaborator

Ha I misread you, then let's rather say drop 3.6, keep 3.7 on ubuntu, 3.8 windows and 3.9 mac?

@hugovk
Copy link
Member

hugovk commented Sep 15, 2022

Please see PR #453

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.

None yet

4 participants