bug#34882: [PATCH] Update to Pandas, enable Excel writer support

[Maxim Cournoyer]

Hi Marius, and thanks for having a look!

Marius Bakke <address@hidden> writes:

> Hello Maxim,
>
> Overall LGTM, some comments inline.
>
> [...]
>
>> +(define-public python-et-xmlfile
>> +  (package
>> +    (name "python-et-xmlfile")
>> +    (version "1.0.1")
>> +    (source
>> +      (origin
>> +        (method url-fetch)
>> +        (uri (pypi-uri "et_xmlfile" version))
>> +        (sha256
>> +          (base32
>> +            "0nrkhcb6jdrlb6pwkvd4rycw34y3s931hjf409ij9xkjsli9fkb1"))))
>> +    (build-system python-build-system)
>> +    (arguments
>> +     `(#:phases (modify-phases %standard-phases
>> +                  (replace 'check
>> +                    (lambda _
>> +                      (invoke "pytest"))))))
>> +    (native-inputs
>> +     `(("python-pytest" ,python-pytest)
>> +       ("python-lxml" ,python-lxml)))
>
> Should python-lxml be a propagated-input?

No, otherwise this package would be pretty pointless, as it aims to be
a "low memory implementation of a component of lxml" :-). The lxml
dependency is used in the test suite (I'm guessing to validate that both
implementations' behaviors match).

>
>> +    (home-page
>> +      "https://bitbucket.org/openpyxl/et_xmlfile";)
>> +    (synopsis
>> +      "Low memory implementation of @code{lxml.xmlfile}")
>
> Please remove the extra newlines in these patches.

Done.

>> +    (description
>> +      "This Python library is based upon the @code{xmlfile} module
>> +from @code{lxml}.  It aims to provide a low memory, compatible
>> implementation
>> +of @code{xmlfile}.")
>> +    (license license:expat)))
>
> [...]
>
>> +(define-public python-openpyxl
>> +  (package
>> +    (name "python-openpyxl")
>> +    (version "2.6.0")
>> +    (source
>> +     (origin
>> +       (method hg-fetch)
>> +       (uri (hg-reference
>> +             (url "https://bitbucket.org/openpyxl/openpyxl";)
>> +             (changeset version)))
>> +       (file-name (string-append name "-" version "-checkout"))
>> +       (sha256
>> +        (base32
>> +         "1x47ngn7ybaqdbvg90c8h2x0j6yfdfj25gjfinp2w5rf62gsany7"))))
>
> Can you leave a comment about why we take it from this repository
> instead of PyPi?

Done. The reason is that the tests are missing from the PyPI
release.

>> +    (native-inputs
>> +     `(("python-lxml" ,python-lxml)
>
> Why is python-lxml a native-input?

Here also it is a test dependency. lxml is an optional backend. I've
moved the existing comment ("     ;; For the test suite.") above this
native-input as well.

>> +       ;; For the test suite.
>> +       ("python-pillow" ,python-pillow)
>> +       ("python-pytest" ,python-pytest)))
>> +    (propagated-inputs
>> +     `(("python-et-xmlfile" ,python-et-xmlfile)
>> +       ("python-jdcal" ,python-jdcal)))
>> +    (home-page "https://openpyxl.readthedocs.io";)
>> +    (synopsis
>> +     "Python library to read/write Excel 2010 XLSX/XLSM files")
>> +    (description
>> + "This Python library allows reading and writing to the Excel XLSX,
>> XLSM,
>> +XLTX and XLTM file formats that are defined by the Office Open XML
>> (OOXML)
>> +standard.")
>> +    (license license:expat)))
>
> [...]
>
>> From ad1f0efe4a5c3d28ee9d7e2e5da275721af9e172 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <address@hidden>
>> Date: Sat, 9 Feb 2019 00:25:51 -0500
>> Subject: [PATCH 5/5] gnu: python-pandas: Update to 0.24.2.
>>
>> * gnu/packages/python-xyz.scm (python-pandas): Update to 0.24.2.
>> [phases]{patch-which}: Add phase.
>> [inputs]: Add WHICH.
>> ---
>>  gnu/packages/python-xyz.scm | 65 ++++++++++++++++++++++---------------
>>  1 file changed, 38 insertions(+), 27 deletions(-)
>>
>> diff --git a/gnu/packages/python-xyz.scm b/gnu/packages/python-xyz.scm
>> index 321c881f4d..bbf1403758 100644
>> --- a/gnu/packages/python-xyz.scm
>> +++ b/gnu/packages/python-xyz.scm
>> @@ -1014,56 +1014,67 @@ human-friendly syntax.")
>>  (define-public python-pandas
>>    (package
>>      (name "python-pandas")
>> -    (version "0.23.4")
>> +    (version "0.24.2")
>>      (source
>>       (origin
>>         (method url-fetch)
>>         (uri (pypi-uri "pandas" version))
>>         (sha256
>> -        (base32 "1x54pd7hr3y7qahx6b5bf2wzj54xvl8r3s1h4pl254pnmi3wl92v"))))
>> +        (base32 "18imlm8xbhcbwy4wa957a1fkamrcb0z988z006jpfda3ki09z4ag"))))
>>      (build-system python-build-system)
>>      (arguments
>>       `(#:modules ((guix build utils)
>>                    (guix build python-build-system)
>>                    (ice-9 ftw)
>>                    (srfi srfi-26))
>> -       #:phases (modify-phases %standard-phases
>> -                  (replace 'check
>> -                    (lambda _
>> -                      (let ((build-directory
>> -                             (string-append
>> -                              (getcwd) "/build/"
>> -                              (car (scandir "build"
>> -                                            (cut string-prefix? "lib." 
>> <>))))))
>> -                        ;; Disable the "strict data files" option which 
>> causes
>> -                        ;; the build to error out if required data files 
>> are not
>> -                        ;; available (as is the case with PyPI archives).
>> -                        (substitute* "setup.cfg"
>> -                          (("addopts = --strict-data-files") "addopts = "))
>> -                        (with-directory-excursion build-directory
>> -                          ;; Delete tests that require "moto" which is not 
>> yet in Guix.
>> -                          (for-each delete-file
>> -                                    '("pandas/tests/io/conftest.py"
>> -                                      
>> "pandas/tests/io/json/test_compression.py"
>> -                                      
>> "pandas/tests/io/parser/test_network.py"
>> -                                      "pandas/tests/io/test_parquet.py"))
>> -                          (invoke "pytest" "-vv" "pandas" "--skip-slow"
>> -                                  "--skip-network" "-k"
>> -                                  ;; XXX: Due to the deleted tests above.
>> -                                  "not test_read_s3_jsonl"))))))))
>> +       #:phases
>> +       (modify-phases %standard-phases
>> +         (add-after 'unpack 'patch-which
>> +           (lambda* (#:key inputs #:allow-other-keys)
>> +             (let ((which (assoc-ref inputs "which")))
>> +               (substitute* "pandas/io/clipboard/__init__.py"
>> +                 (("^CHECK_CMD = .*")
>> +                  (string-append "CHECK_CMD = \"" which "\"\n"))))
>> +             #t))
>> +         (replace 'check
>> +           (lambda _
>> +             (let ((build-directory
>> +                    (string-append
>> +                     (getcwd) "/build/"
>> +                     (car (scandir "build"
>> +                                   (cut string-prefix? "lib." <>))))))
>> +               ;; Disable the "strict data files" option which causes
>> +               ;; the build to error out if required data files are not
>> +               ;; available (as is the case with PyPI archives).
>> +               (substitute* "setup.cfg"
>> +                 (("addopts = --strict-data-files") "addopts = "))
>> +               (with-directory-excursion build-directory
>> +                 ;; Delete tests that require "moto" which is not yet in 
>> Guix.
>> +                 (for-each delete-file
>> +                           '("pandas/tests/io/conftest.py"
>> +                             "pandas/tests/io/json/test_compression.py"
>> +                             "pandas/tests/io/parser/test_network.py"
>> +                             "pandas/tests/io/test_parquet.py"))
>> +                 (invoke "pytest" "-vv" "pandas" "--skip-slow"
>> +                         "--skip-network" "-k"
>> +                         ;; XXX: Due to the deleted tests above.
>> +                         "not test_read_s3_jsonl"))))))))
>
> LGTM, although I'd prefer not to reindent the phases section.  It makes
> the patch harder to read, and I prefer the "deep" indentation for
> logically separate chunks of code anyway (though I am probably in the
> minority here..).  YMMV!

While I loathe any "deep" indentation, I've reverted my indentation
change here as it was a bit gratuitous (I needn't struggle to fit into
the 80 chars guideline).

> Thanks!

I pushed this change as c0d43f6223 with modifications based on your feedback.

Thank you!

Maxim