diff mbox series

[bug#42082,1/6] gnu: Add python-covdefaults.

Message ID 20200627154645.64264-1-monego@posteo.net
State Accepted
Headers show
Series [bug#42082,1/6] gnu: Add python-covdefaults. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job

Commit Message

Vinicius Monego June 27, 2020, 3:46 p.m. UTC
* gnu/packages/check.scm (python-covdefaults): New variable.
---
 gnu/packages/check.scm | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Marius Bakke July 20, 2020, 9:13 p.m. UTC | #1
Hi Vinicius,

Sorry for the slow response.  The series look great overall, but I have
a few nitpicks.  :-)

Vinicius Monego <monego@posteo.net> writes:

> * gnu/packages/check.scm (python-covdefaults): New variable.

We now have a 'python-check.scm' which is preferable to check.scm.  Can
you add this library there instead?

Ideally we'd move the current Python test libraries over too, but that's
another issue.

[...]
  
> +(define-public python-covdefaults
> +  (package
> +    (name "python-covdefaults")
> +    (version "1.1.0")
> +    (source
> +     (origin
> +       ;; The PyPI tarball does not include tests.
> +       (method git-fetch)
> +       (uri (git-reference
> +             (url "https://github.com/asottile/covdefaults")
> +             (commit (string-append "v" version))))
> +       (file-name (git-file-name name version))
> +       (sha256
> +        (base32 "11a24c0wzv01n55fy4kdpnyqna4m9k0mp58kmhiaks34xw4k37hq"))))
> +    (build-system python-build-system)
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (replace 'check
> +           (lambda _
> +             (invoke "pytest" "-vv")
> +             #t)))))

Note: invoke returns #t on success, so no need for the explicit return
(though it does not hurt).

> +    (propagated-inputs
> +     `(("python-coverage" ,python-coverage)
> +       ("python-pytest" ,python-pytest)))

Should pytest be propagated here?

Also, as this package is a plugin for coverage, maybe it should not
propagate coverage either.  The reason is that propagating prevents it
from being (easily) used with other versions of coverage.  Let's say
that a package needs a newer coverage than the default in Guix + this
plugin, propagation here would bring in the wrong coverage version.

> +    (home-page "https://github.com/asottile/covdefaults")
> +    (synopsis "Coverage plugin to provide sensible default settings")
> +    (description
> +     "Covdefaults is a coverage plugin to provide sensible default settings.")

I know it's a lot to ask :-) but can you try to expand on how it differs
from the apparently unreasonable defaults in coverage?  "Sensible" also
borders on "marketing speak", perhaps "opinionated" is a better term.

This was more text than I intended for an otherwise good-looking patch!
Will comment on the others too.
Vinicius Monego July 21, 2020, 6:28 p.m. UTC | #2
Em seg, 2020-07-20 às 23:13 +0200, Marius Bakke escreveu:
> Hi Vinicius,

Hello Marius,

Thanks for the tips!

> We now have a 'python-check.scm' which is preferable to
> check.scm.  Can
> you add this library there instead?

Sure. I overlooked that file. I sent another series of 6 pytest patches
before this, and will send a v2 for that as well.

> Should pytest be propagated here?
> 
> Also, as this package is a plugin for coverage, maybe it should not
> propagate coverage either.  The reason is that propagating prevents
> it
> from being (easily) used with other versions of coverage.  Let's say
> that a package needs a newer coverage than the default in Guix + this
> plugin, propagation here would bring in the wrong coverage version.

Oh, I thought that the pytest binary should be available as an
executable for the plugin to be used, similar to how APT has pytest as
a dependency for plugins. Maybe it's just me thinking in terms of
traditional package management.

The PyPI importer will propagate pytest by default, and it's common to
see it propagated in other plugins in check.scm. But now I see that
many plugins do not propagate it. Should pytest not be an input at all
(unless the plugin uses it for its own tests, in which case it should
be a native input)?

> I know it's a lot to ask :-) but can you try to expand on how it
> differs
> from the apparently unreasonable defaults in coverage?  "Sensible"
> also
> borders on "marketing speak", perhaps "opinionated" is a better term.

That's a slight rewording of the author's description in the
repository, and that's the only verbal information I could find about
the project. The differences can be seen in the README and in the
source code, but I don't know what to write about them, or even why the
changes are 'more sensible'.
Marius Bakke July 21, 2020, 9:48 p.m. UTC | #3
Vinicius Monego <monego@posteo.net> writes:

> Em seg, 2020-07-20 às 23:13 +0200, Marius Bakke escreveu:
>> Hi Vinicius,
>
> Hello Marius,
>
> Thanks for the tips!
>
>> We now have a 'python-check.scm' which is preferable to
>> check.scm.  Can
>> you add this library there instead?
>
> Sure. I overlooked that file. I sent another series of 6 pytest patches
> before this, and will send a v2 for that as well.

Excellent, thank you.

>> Should pytest be propagated here?
>> 
>> Also, as this package is a plugin for coverage, maybe it should not
>> propagate coverage either.  The reason is that propagating prevents
>> it
>> from being (easily) used with other versions of coverage.  Let's say
>> that a package needs a newer coverage than the default in Guix + this
>> plugin, propagation here would bring in the wrong coverage version.
>
> Oh, I thought that the pytest binary should be available as an
> executable for the plugin to be used, similar to how APT has pytest as
> a dependency for plugins. Maybe it's just me thinking in terms of
> traditional package management.
>
> The PyPI importer will propagate pytest by default, and it's common to
> see it propagated in other plugins in check.scm. But now I see that
> many plugins do not propagate it. Should pytest not be an input at all
> (unless the plugin uses it for its own tests, in which case it should
> be a native input)?

There are differing opinions on this matter :-) I would say that pytest
should _not_ be propagated in this case, so that the plugin can be used
with custom versions of pytest.  Others might say that the plugin is
useless without pytest, and so it _should_ be propagated.

I'll leave the final say to you.  :-)

>> I know it's a lot to ask :-) but can you try to expand on how it
>> differs
>> from the apparently unreasonable defaults in coverage?  "Sensible"
>> also
>> borders on "marketing speak", perhaps "opinionated" is a better term.
>
> That's a slight rewording of the author's description in the
> repository, and that's the only verbal information I could find about
> the project. The differences can be seen in the README and in the
> source code, but I don't know what to write about them, or even why the
> changes are 'more sensible'.

Right, not great.  We should at least try and stay neutral (see the
"Synopses and Descriptions" section of the manual).  So
s/sensible/opinionated/ is good enough for me.
Brett Gilio July 25, 2020, 2:21 a.m. UTC | #4
merge 42082 42083 42084 42085 42086 42087
diff mbox series

Patch

diff --git a/gnu/packages/check.scm b/gnu/packages/check.scm
index c71edc7655..c2435a78af 100644
--- a/gnu/packages/check.scm
+++ b/gnu/packages/check.scm
@@ -1081,6 +1081,37 @@  same arguments.")
        `(("python2-mock" ,python2-mock)
          ,@(package-propagated-inputs base))))))
 
+(define-public python-covdefaults
+  (package
+    (name "python-covdefaults")
+    (version "1.1.0")
+    (source
+     (origin
+       ;; The PyPI tarball does not include tests.
+       (method git-fetch)
+       (uri (git-reference
+             (url "https://github.com/asottile/covdefaults")
+             (commit (string-append "v" version))))
+       (file-name (git-file-name name version))
+       (sha256
+        (base32 "11a24c0wzv01n55fy4kdpnyqna4m9k0mp58kmhiaks34xw4k37hq"))))
+    (build-system python-build-system)
+    (arguments
+     `(#:phases
+       (modify-phases %standard-phases
+         (replace 'check
+           (lambda _
+             (invoke "pytest" "-vv")
+             #t)))))
+    (propagated-inputs
+     `(("python-coverage" ,python-coverage)
+       ("python-pytest" ,python-pytest)))
+    (home-page "https://github.com/asottile/covdefaults")
+    (synopsis "Coverage plugin to provide sensible default settings")
+    (description
+     "Covdefaults is a coverage plugin to provide sensible default settings.")
+    (license license:expat)))
+
 (define-public python-pytest-xdist
   (package
     (name "python-pytest-xdist")