From debbugs-submit-bounces@debbugs.gnu.org Wed Apr 22 11:53:32 2020 Received: (at 40757) by debbugs.gnu.org; 22 Apr 2020 15:53:32 +0000 Received: from localhost ([127.0.0.1]:52988 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jRHgm-0004Mt-TU for submit@debbugs.gnu.org; Wed, 22 Apr 2020 11:53:32 -0400 Received: from minsky.hcoop.net ([104.248.1.95]:44592) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jRHgi-0004MX-FW for 40757@debbugs.gnu.org; Wed, 22 Apr 2020 11:53:27 -0400 Received: from marsh.hcoop.net ([45.55.52.66]) by minsky.hcoop.net with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1jRHgc-0004BK-Ew; Wed, 22 Apr 2020 11:53:18 -0400 Date: Wed, 22 Apr 2020 11:53:18 -0400 (EDT) From: Jack Hill X-X-Sender: jackhill@marsh.hcoop.net To: Ryan Prior Subject: Re: [bug#40757] New package: VisiData In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 40757 Cc: 40757@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) Ryan, On Wed, 22 Apr 2020, Ryan Prior via Guix-patches via wrote: > Hi Guix! This patch adds a package for VisiData. > > VisiData is an interactive multitool for tabular data. It combines the clarity of a > spreadsheet, the efficiency of the terminal, and the power of Python, into a > lightweight utility which can handle millions of rows with ease. > https://www.visidata.org/, GPLv3 Thanks for the patch! This looks like some cool software. I'm not an expert reviewer, but I'd like to try to help by offering the following suggestions for improvement. >diff --git a/gnu/packages/visidata.scm b/gnu/packages/visidata.scm >new file mode 100644 >index 0000000000..d9df94dc9b >--- /dev/null >+++ b/gnu/packages/visidata.scm When adding new modules, please also add the file to gnu/local.mk >+(define-module (visidata) The module name should be (gnu packages visidata) to match the filesystem path. It might be helpful to catch problems like this to try to apply your patch apply your patch to a guix source checkout, and try building your package from there. The manual has some documentation about that, , but I can answer questions as well. >+(define-public visidata >+ (package >+ (name "visidata") >+ (version "1.5.2") >+ (source (origin >+ (method url-fetch) >+ (uri (string-append "https://github.com/saulpw/visidata/archive/v" version ".tar.gz")) The GitHub archive URIs are not stable and could result in a different hash in the future. Often in these cases we use git-fetch, but since visidata is available on pypi, I would recommend (uri (pypi-uri "visidata" version)) >+ (sha256 (base32 "0h7hq6bnc8svkcc9995kkmgcb9n5qgm85rsshzzdicmg9rg3ymhi")))) >+ (build-system python-build-system) >+ (arguments '(#:tests? #f)) >+ ;; Tests disabled because they are not packaged with the source tarball. >+ ;; View test status here: https://circleci.com/gh/saulpw/visidata/tree/stable >+ ;; Upstream suggests tests will be packaged with tarball around 2.0 release. Nitpick: I would prefer to have the comment before the code. I think the line about circleci could be removed, but the other two seem good. >+ (native-inputs >+ `(("python-dateutil" ,python-dateutil) >+ ("python-fonttools" ,python-fonttools) >+ ("python-h5py" ,python-h5py) >+ ("python-lxml" ,python-lxml) >+ ("python-openpyxl" ,python-openpyxl) >+ ("python-psycopg2" ,python-psycopg2) >+ ("python-pyyaml" ,python-pyyaml) >+ ("python-requests" ,python-requests) >+ ("python-xlrd" ,python-xlrd) >+ ("python-pandas" ,python-pandas))) Are these all native inputs (required to make the build system run)? I expect that some of them could be moved to inputs (run-tine dependencies). >+ (synopsis "Visidata: A terminal spreadsheet multitool for discovering and arranging data") guix lint reports that the synopsis should not start with the package name. In this case, I think it could be changed to, "Terminal spreadsheet for discovering and arranging data". >+ (description >+ "VisiData is an interactive multitool for tabular data. It combines the Please use two spaces between sentences in the description. >+clarity of a spreadsheet, the efficiency of the terminal, and the power of >+Python, into a lightweight utility which can handle millions of rows with ease.") Nitpick: "with ease" sounds like a marketing term, and I believe we can leave it out here. >+ (home-page "https://www.visidata.org/") >+ (license gpl3))) In other modules, we import licenses with the license: prefix, so we can refer to licenses as like license:gpl3. It might be nice to do that here for consistency as well. The vdtui.py file is under the expat license, so we should record that here. With these changes the license field would become something like: (license (list license:gpl3 license:expat)) ; visidata/vdtui.py Thanks and all the best, Jack