`needs_nox` in noxfile #210

Paulius-Maruska · 2019-07-05T11:10:47Z

Hello, in an attempt to familiarize myself with nox code, I managed to implement #99

theacodes · 2019-07-06T03:42:23Z

@crwilcox @dhermes @stsewd do any of you have a moment to look at this? I don't have the bandwidth right now.

@Paulius-Maruska thank you for submitting this!

stsewd

Nice approach!

docs/config.rst

nox/_version.py

Paulius-Maruska · 2019-07-08T21:22:43Z

I have updated the PR, and also rebased it on the latest master branch.

crwilcox

Thank you for the PR. I left a couple of comments that I would like follow up on, but overall this looks good.

crwilcox · 2019-07-08T21:50:11Z

docs/config.rst

+
+    import nox
+
+    needs_nox = '2019.5.30'


echoing a comment from @dhermes on the issue. This noxfile.py would result in pylint throwing lint_test.py:3:0: C0103: Constant name "needs_nox" doesn't conform to UPPER_CASE naming style (invalid-name)

Any objections to changing this to a constant?

crwilcox · 2019-07-08T21:59:49Z

tests/resources/noxfile_future.py

+# limitations under the License.
+
+# future version, to make sure error is logged and returned
+needs_nox = "3001.1.1"


It seems a bit 'heavy' to have multiple resources files to validate this. Could these be generated at test time? How are these unique to the tests in test_version?

crwilcox · 2019-07-08T22:18:23Z

tests/test_tasks.py

+    assert tasks.load_nox_module(config) == 2
+
+
+def test_load_nox_module_returns_2_when_noxfile_does_not_even_import():


It isn't clear to me what this validates. Is this validating the ordering of errors? That we will mention that we have an invalid arg to the decorator before the version?

crwilcox · 2019-07-08T22:20:08Z

tests/test__version.py

+    assert needed is None
+
+
+def test_is_version_sufficient_returns_true_for_lower_version():


should there be a case for exact match?

crwilcox · 2019-07-08T22:37:07Z

docs/config.rst

+Minimum required nox version
+----------------------------
+
+Minimum required Nox version can be specified in your ``noxfile.py`` file by simply defining


nitpick: may make sense to drop the word 'simply' as there can be negative feelings about that verbiage in prose.

Suggested change

Minimum required Nox version can be specified in your ``noxfile.py`` file by simply defining

Minimum required Nox version can be specified in your ``noxfile.py`` file by defining

theacodes · 2019-07-09T03:32:40Z

In terms of placating linters and continuing to be somewhat idiomatic, what do y'all think of making needs_version a member of the nox module?

import nox

nox.needs_version = ">=2018.7.8"
nox.options.sessions = ["lint", "tests-3.6"]

@nox.session
def lint(s):
    ...

...

crwilcox · 2019-07-09T03:46:20Z

I think adding this to the module would make sense. It also may be more discoverable via language intelligence services in IDEs. Though my big sticking point is matching PyLint, just to avoid the bug that will follow if this doesn't :D

stsewd · 2019-07-09T04:41:18Z

nox/_version.py

+    if needed_version and needed_version > nox_version():
+        logger.error(
+            "Noxfile {} needs at least Nox {} and therefore it cannot be used "
+            "with this version.".format(noxfile, str(needed_version))


This message could include the current nox version too

stsewd · 2019-07-09T04:51:58Z

nox/_version.py

+                return needs_version
+
+
+def needs_nox(noxfile, noxfile_module=None):


Np: this function is doing two things. And only using one arg. If noxfile and noxfile module are given, noxfile module is only used

Paulius-Maruska · 2019-07-14T08:03:29Z

I'm sorry for the silence this past week - I was attending the EuroPython2019 conference and was quite busy with the various activities here. :)

I will try to address the issues / respond next week as early as I can.

theacodes · 2019-07-15T05:36:38Z

Take your time, and thanks for keeping us updated. 🙂

…

On Sun, Jul 14, 2019, 1:03 AM Paulius Maruška ***@***.***> wrote: I'm sorry for the silence this past week - I was attending the EuroPython2019 conference and was quite busy with the various activities here. :) I will try to address the issues / respond next week as early as I can. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#210>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB5I46QCZA6KUTDVRSXEATP7LMVDANCNFSM4H6KLOJQ> .

theacodes · 2021-02-11T21:16:26Z

I'm going to close this PR due to inactivity. That's not to say I'm not interested in merging it! If you're still up for it just reply and I'll re-open the PR. :)

stsewd reviewed Jul 7, 2019

View reviewed changes

docs/config.rst Outdated Show resolved Hide resolved

nox/_version.py Outdated Show resolved Hide resolved

nox/_version.py Show resolved Hide resolved

Paulius-Maruska added 4 commits July 8, 2019 23:14

Added helper functions for needs_nox checking

1706b17

Nox version is now checked during module loading

31f5492

Added documentation for needs_nox

d8f771c

Fixes according to @stsewd comments

bda165d

Paulius-Maruska force-pushed the feature/needs-nox-in-noxfile branch from 6c6d27f to bda165d Compare July 8, 2019 21:18

crwilcox reviewed Jul 8, 2019

View reviewed changes

stsewd reviewed Jul 9, 2019

View reviewed changes

theacodes added the awaiting submitter action label Aug 1, 2019

theacodes closed this Feb 11, 2021

cjolowicz mentioned this pull request Feb 17, 2021

Add nox.needs_version to specify Nox version requirements #388

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`needs_nox` in noxfile #210

`needs_nox` in noxfile #210

Paulius-Maruska commented Jul 5, 2019

theacodes commented Jul 6, 2019

stsewd left a comment

Paulius-Maruska commented Jul 8, 2019

crwilcox left a comment

crwilcox Jul 8, 2019

crwilcox Jul 8, 2019

crwilcox Jul 8, 2019

crwilcox Jul 8, 2019

crwilcox Jul 8, 2019

theacodes commented Jul 9, 2019

crwilcox commented Jul 9, 2019

stsewd Jul 9, 2019

stsewd Jul 9, 2019

Paulius-Maruska commented Jul 14, 2019

theacodes commented Jul 15, 2019 via email

theacodes commented Feb 11, 2021

		assert tasks.load_nox_module(config) == 2


		def test_load_nox_module_returns_2_when_noxfile_does_not_even_import():

		assert needed is None


		def test_is_version_sufficient_returns_true_for_lower_version():

	Minimum required Nox version can be specified in your ``noxfile.py`` file by simply defining
	Minimum required Nox version can be specified in your ``noxfile.py`` file by defining

		return needs_version


		def needs_nox(noxfile, noxfile_module=None):

needs_nox in noxfile #210

needs_nox in noxfile #210

Conversation

Paulius-Maruska commented Jul 5, 2019

theacodes commented Jul 6, 2019

stsewd left a comment

Choose a reason for hiding this comment

Paulius-Maruska commented Jul 8, 2019

crwilcox left a comment

Choose a reason for hiding this comment

crwilcox Jul 8, 2019

Choose a reason for hiding this comment

crwilcox Jul 8, 2019

Choose a reason for hiding this comment

crwilcox Jul 8, 2019

Choose a reason for hiding this comment

crwilcox Jul 8, 2019

Choose a reason for hiding this comment

crwilcox Jul 8, 2019

Choose a reason for hiding this comment

theacodes commented Jul 9, 2019

crwilcox commented Jul 9, 2019

stsewd Jul 9, 2019

Choose a reason for hiding this comment

stsewd Jul 9, 2019

Choose a reason for hiding this comment

Paulius-Maruska commented Jul 14, 2019

theacodes commented Jul 15, 2019 via email

theacodes commented Feb 11, 2021

`needs_nox` in noxfile #210

`needs_nox` in noxfile #210