add Python3.9 and Django 2.2 support, drop Python2.7 and Django 1.11support #13

imaxyz · 2022-03-04T07:53:51Z

目的

bpmailerのユニットテストが、Python 3.6, Python 3.9. Django 2.2の環境でpassするようにすること
Python 2.7とDjango 1.11のサポートを止めること

作業内容

toxがfailしてしまったため、Djangoのテストランナーに渡す単体テストのPathの書き方を修正しました。
Travis CIの使用を止めて、Github Actionsでtoxを実行するように変更しました。
- Travis CIの設定ファイルを削除しました。
- tox.iniに、Github Actionsの設定を追記しました。
ローカル環境で単体テストする際に使う意図でrequirements.txtを新規作成しました。
READMEの書式をmarkdownからreStructuredTxtに変更しました。

レビューしてほしいところ

.github/workflows/tests.yml の書き方は問題ないか
tox.iniの書き方に不足はないか
コメントに違和感はないか
バージョン1.2をリリースするにあたって、何か他に変更が必要な点はあるか

https://github.com/django/django/blob/e541f2d05b88e58c18b82b622aacc38d670eb5f6/django/test/runner.py#L618

imaxyz · 2022-03-09T02:16:37Z

tests.py

+    test_runner = TestRunner()
+
+    # set 'bpmailer unit test path' and run the unit test
+    failures = test_runner.run_tests(['beproud.django.mailer.tests'])


(Djangoのバージョンが変わったためか?) toxが通らなくなっていたので、Djangoのrun_tests()メソッドのドキュメントを参考に、テストモジュールへのPathを修正しました。

Test labels should be dotted Python paths to test modules, test classes, or test methods.

https://github.com/django/django/blob/e541f2d05b88e58c18b82b622aacc38d670eb5f6/django/test/runner.py#L618

原因はsetuptoolsの更新のためのようです。変更自体はまずはこれでよさそう。

Avoid deprecated load_module() in pkg_resources namespace delaration pypa/setuptools#2523

https://setuptools.pypa.io/en/latest/history.html#v51-2-0 →同様のエラーが発生する

https://setuptools.pypa.io/en/latest/history.html#v51-1-2 →発生しない

このPRで対応するかは任せますが、 python setup.py test コマンド自体が非推奨になっているのでpytestに置き換えた方がよさそうです。 https://setuptools.pypa.io/en/latest/userguide/commands.html?highlight=test_suite#test-build-package-and-run-a-unittest-suite

MUST: レビューのために、この修正の妥当性の説明が欲しいです。

(Djangoのバージョンが変わったためか?)

確信はないけどこうしたら動いた、ということでしょうか。

現在 bpmailerのtoxがfailする原因について

@kashewnuts

頂いたコメントに関して、py36, py39共に、bpmailerのtox.iniの testenv - deps の項目に、次の行を追加することでtoxから実行する単体テストがpassすることを確認しました。

setuptools<=51.1.2

※ この改修がsetuptoolsの51.1.2の次のバージョンである51.2.0でマージされたことも確認しました

このことから、今回の作業でbpmailerのtoxがfailしている原因は、setuptoolsが、bpmailerをpy36対応した時の作業の時と比べてバージョンアップしていて、動作が変わっていたことが原因と思いました。

python setup.py test コマンドが非推奨の件

@kashewnuts

#14 でissueにしました。別PRで対応できればと思います。

今回の改修方法の妥当性について

@shimizukawa

(Djangoのバージョンが変わったためか?)

最初上記のようにコメントしたのですが、上述のsetuptoolsの調査結果から、Djnagoのバージョンは関係なさそうです。

setuptoolsの改修で使われているメソッドの差分について調べられたらベストなのですが、まだ調べられていません。

改修方法の妥当性について、確信は無いのですが、bpmailerが使用するバージョンのDjnagoのrun_tests()のコメントに

Test labels should be dotted Python paths to test modules, test classes, or test methods.
意訳: ラベルは「テストモジュールか、テストクラスか、テストメソッド」にすべき

と書かれているのに対して、テストモジュールの上位のパッケージ名(mailer)を指定していることから、Djangoが想定するパラメータ仕様に近づける意味で、適切ではと考えました。

# before test_runner.run_tests(['beproud.django.mailer']) # after test_runner.run_tests(['beproud.django.mailer.tests'])

あと、tox.iniのdeps項目に、setuptoolsのバージョンを指定すべきか？を考えました。

現在の改修方法だと、setuptoolsのバージョンが51.2.0未満でも51.2.0以上でもtoxがpassしました。
なので、tox.iniにsetuptoolsのバージョン指定を記述する必要ないと考えました。

OKです。別PRでtest.pyを削除するのであれば、問題ないです。

kashewnuts · 2022-03-10T07:02:12Z

README.md

@@ -2,6 +2,6 @@



SHOULD: travisの結果を見ているのでGitHub Action版に変更してください。

アイコンをGithub Actionsの結果を表示するように修正しました。

アイコンを選択した際に、https://github.com/beproud/bpmailer/actions に遷移させたかったので、rst形式に変更しました。

補足

Markdownだとsvg画像へリンクする記述方法しか見当たりませんでした。

https://docs.github.com/ja/actions/monitoring-and-troubleshooting-workflows/adding-a-workflow-status-badge

バッチアイコンを選択して、バッチの画像をダウンロードするよりか、より情報が多いWebページに遷移した方が良いと考え、rstに変更してみました

kemu3007 · 2022-03-11T05:52:07Z

.github/workflows/tests.yml

+    strategy:
+      max-parallel: 4
+      matrix:
+        python-version: ['3.6', '3.9']


3.6はすでにEOLです。3.7に修正して良さそう

@kemu3007 このbpmailerを使いたいプロジェクトがまだPython3.6を使っているため、現時点ではPython3.6対応は残したい感じです。

shimizukawa

レビューしました

shimizukawa · 2022-03-11T05:21:28Z

.github/workflows/tests.yml

+
+    # 並列して実行する各ジョブのPythonバージョン
+    strategy:
+      max-parallel: 4


ASK: これ要りますか？（制限しているのを初めて見た）

無指定にして、実行環境で使えるだけ使えばよいのでは。

shimizukawa · 2022-03-11T05:23:23Z

requirements.txt

@@ -0,0 +1,5 @@
+# toxを使用せず、直接 python tests.py で単体テスト実行する際に pip installするパッケージ一覧です。


SHOULD: ファイル名を requirements-test.txt 等にして、テストでのみ必要という意味を持たせて欲しい。

ファイル名を修正しました。

shimizukawa · 2022-03-11T05:24:45Z

tests.py

+    test_runner = TestRunner()
+
+    # set 'bpmailer unit test path' and run the unit test
+    failures = test_runner.run_tests(['beproud.django.mailer.tests'])


OKです。別PRでtest.pyを削除するのであれば、問題ないです。

shimizukawa · 2022-03-11T05:51:59Z

tox.ini

-
-[travis:env]
-DJANGO =
-  1.11: django111


MUST: 1.11をドロップしたのであれば、setup.pyも更新してください。

bpmailer/setup.py

Line 63 in e44f6b4

install_requires=['Django>=1.11', 'six'],

以下、ついでに依頼です。
classifiers も更新お願いします。

bpmailer/setup.py

Lines 51 to 59 in e44f6b4

classifiers=[

'Development Status :: 3 - Alpha',

'Environment :: Plugins',

'Framework :: Django',

'Intended Audience :: Developers',

'License :: OSI Approved :: BSD License',

'Programming Language :: Python',

'Topic :: Software Development :: Libraries :: Python Modules',

],

参考記述

https://github.com/jazzband/django-redshift-backend/blob/6b862eca2e1c6d5f66fe0dd4e1646be573256f59/setup.cfg#L15-L25

setup.pyの記述内容を修正しました。

shimizukawa

LGTM + 1つ「できれば」依頼

shimizukawa · 2022-03-11T06:18:27Z

setup.py

        'Topic :: Software Development :: Libraries :: Python Modules',
    ],
    include_package_data=True,
    packages=find_packages(),
    namespace_packages=['beproud', 'beproud.django'],
-    install_requires=['Django>=1.11', 'six'],
+    install_requires=['Django>=2.2', 'six'],


すみません、もう1個思い出しました。
できれば対応お願いします。

Py2 drop するのであれば、 python_requires で3.6以上を指定して下さい。
https://packaging.python.org/en/latest/guides/distributing-packages-using-setuptools/#python-requires

例
https://github.com/jazzband/django-redshift-backend/blob/master/setup.cfg#L36

追記しました

kashewnuts

LGTM with nits. 直近の作業には影響しないレベルだと思います。

kashewnuts · 2022-03-11T06:13:24Z

.github/workflows/tests.yml

+    # Github Actionsからtoxを実行
+    - name: Test with tox
+      run: |
+        tox -l


マトリクス対応したので -l は不要な認識です。削除してください。 https://github.com/beproud/bpmailer/pull/13/files#r824402013

削除しました

kashewnuts · 2022-03-11T06:30:40Z

requirements-test.txt

@@ -0,0 +1,5 @@
+# toxを使用せず、直接 python tests.py で単体テスト実行する際に pip installするパッケージ一覧です。


IMO: この後Django3.2対応もあると思うので、Django2.2固定のこのファイルは削除することになると思います。なのでこのPRでは一旦よいかもしれませんが、後々は削除したほうが第三者が混乱しなくてよさそうです。

なるほどです。ryです。今回は残しておきます。

setup.py

kashewnuts · 2022-03-11T06:33:39Z

tox.ini

@@ -1,29 +1,21 @@
 # Requires tox > 1.8

 [tox]
-envlist = py27-django111, py36-django{111,22}
+envlist = py36-django22, py39-django22


nits: 書き方を省略できます #13 (comment)

suggestionを見落としていました。

修正しました。

Co-authored-by: Kashun YOSHIDA <1122795+kashewnuts@users.noreply.github.com>

Python3.9対応の記述を追加

1c7ce2f

imaxyz changed the title ~~[WIP] add Python3.9 + Django 2.2 support~~ add Python3.9 + Django 2.2 support Mar 4, 2022

READMEにPython3.9を追加

d1fd47f

imaxyz marked this pull request as ready for review March 4, 2022 08:34

imaxyz closed this Mar 4, 2022

imaxyz reopened this Mar 4, 2022

imaxyz marked this pull request as draft March 4, 2022 08:41

imaxyz added 8 commits March 8, 2022 15:04

Travis CIの設定を削除。Github Actionsの設定を追加。

62b696c

toxを使わない場合の動作確認

804876d

toxを使わない場合のgithub actions動作確認

fb40940

wip: github actions動作確認

3838514

wip: github actions動作確認

9897f35

wip: github actions動作確認

6ce6c6b

wip: toxで実行するように戻す

f66940e

bpmailerのユニットテストのpathの記述を修正。コメントを追加。

8fcfc5f

https://github.com/django/django/blob/e541f2d05b88e58c18b82b622aacc38d670eb5f6/django/test/runner.py#L618

imaxyz commented Mar 9, 2022

View reviewed changes

imaxyz added 2 commits March 9, 2022 11:55

Readmeに記載されているパッケージをrequirements.txtに記述。各種設定ファイルにコメントを追記。

1b33848

コメント文を日本語に変更

ebc3d14

imaxyz marked this pull request as ready for review March 9, 2022 03:15

imaxyz requested review from ae35bp, mtb-beta, shimizukawa and posbin and removed request for shimizukawa, mtb-beta and ae35bp March 9, 2022 03:18

コメントを修正

3ea2674

kashewnuts reviewed Mar 10, 2022

View reviewed changes

imaxyz added 9 commits March 10, 2022 16:08

Travis CIのビルド結果アイコンを、Github Actionsのワークフロー結果のアイコンに変更

aa6ebde

github actionsのワークフローに、これから実行するtoxの動きを確認するコマンドを追加

733479d

何のためにあるrequirements.txtファイルなのかを説明するコメントを記述しました。

e40af11

Githu Actionsのジョブを並列実行するように修正

6979781

READMEにおけるgithub actionsのリンクを修正

ed5c025

READMEにおけるgithub actionsのリンクを修正

76d2427

READMEにおけるgithub actionsのリンクを修正

fecf1ae

リンク表現の向上のためREADMEの書式をmarkdownからreStructuredTextに変更

76b6b17

Django 1.11を非対応にする

fbe6534

imaxyz requested review from tell-k, shimizukawa and kashewnuts March 11, 2022 05:47

imaxyz changed the title ~~add Python3.9 + Django 2.2 support~~ add Python3.9 + Django 2.2 support, drop Python2.7, Django 1.11support Mar 11, 2022

kemu3007 reviewed Mar 11, 2022

View reviewed changes

imaxyz changed the title ~~add Python3.9 + Django 2.2 support, drop Python2.7, Django 1.11support~~ add Python3.9 and Django 2.2 support, drop Python2.7 and Django 1.11support Mar 11, 2022

shimizukawa requested changes Mar 11, 2022

View reviewed changes

imaxyz added 4 commits March 11, 2022 14:57

Django 1.11のサポートをドロップする旨を反映

46d2807

不要と思われるジョブの制御を削除

848e7f6

ファイル名に意図を込めました

22a2dd6

classifiersの記述を最新の内容に変更

411ef07

imaxyz requested a review from shimizukawa March 11, 2022 06:14

shimizukawa approved these changes Mar 11, 2022

View reviewed changes

パッケージを使用する際に要求するPythonバージョンを指定

a1a425a

kashewnuts approved these changes Mar 11, 2022

View reviewed changes

imaxyz and others added 3 commits March 11, 2022 15:36

tox -l はmasterにマージするワークフローには入れないようにする

6b9b31c

tox.iniの書き方を推敲

ba0fbfe

依存パッケージの記述を追記。

2063bf7

Co-authored-by: Kashun YOSHIDA <1122795+kashewnuts@users.noreply.github.com>

imaxyz merged commit e0afce2 into master Mar 11, 2022

imaxyz deleted the py39-django22 branch March 11, 2022 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Python3.9 and Django 2.2 support, drop Python2.7 and Django 1.11support #13

add Python3.9 and Django 2.2 support, drop Python2.7 and Django 1.11support #13

imaxyz commented Mar 4, 2022 •

edited

imaxyz Mar 9, 2022 •

edited

kashewnuts Mar 10, 2022 •

edited

shimizukawa Mar 10, 2022

imaxyz Mar 11, 2022 •

edited

shimizukawa Mar 11, 2022

kashewnuts Mar 10, 2022

imaxyz Mar 10, 2022 •

edited

imaxyz Mar 11, 2022

kemu3007 Mar 11, 2022

imaxyz Mar 11, 2022

shimizukawa left a comment

shimizukawa Mar 11, 2022

shimizukawa Mar 11, 2022

imaxyz Mar 11, 2022

shimizukawa Mar 11, 2022

shimizukawa Mar 11, 2022

imaxyz Mar 11, 2022

shimizukawa left a comment

shimizukawa Mar 11, 2022

imaxyz Mar 11, 2022

kashewnuts left a comment

kashewnuts Mar 11, 2022

imaxyz Mar 11, 2022

kashewnuts Mar 11, 2022

imaxyz Mar 11, 2022

kashewnuts Mar 11, 2022

imaxyz Mar 11, 2022

		@@ -0,0 +1,5 @@
		# toxを使用せず、直接 python tests.py で単体テスト実行する際に pip installするパッケージ一覧です。

	classifiers=[
	'Development Status :: 3 - Alpha',
	'Environment :: Plugins',
	'Framework :: Django',
	'Intended Audience :: Developers',
	'License :: OSI Approved :: BSD License',
	'Programming Language :: Python',
	'Topic :: Software Development :: Libraries :: Python Modules',
	],

add Python3.9 and Django 2.2 support, drop Python2.7 and Django 1.11support #13

add Python3.9 and Django 2.2 support, drop Python2.7 and Django 1.11support #13

Conversation

imaxyz commented Mar 4, 2022 • edited

目的

作業内容

レビューしてほしいところ

imaxyz Mar 9, 2022 • edited

Choose a reason for hiding this comment

kashewnuts Mar 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imaxyz Mar 11, 2022 • edited

Choose a reason for hiding this comment

現在 bpmailerのtoxがfailする原因について

python setup.py test コマンドが非推奨の件

今回の改修方法の妥当性について

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imaxyz Mar 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

補足

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimizukawa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimizukawa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashewnuts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imaxyz commented Mar 4, 2022 •

edited

imaxyz Mar 9, 2022 •

edited

kashewnuts Mar 10, 2022 •

edited

imaxyz Mar 11, 2022 •

edited

`python setup.py test` コマンドが非推奨の件

imaxyz Mar 10, 2022 •

edited