Bug 1271379 - Review Request: q-text-as-data - Q - Text as Data
Review Request: q-text-as-data - Q - Text as Data
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Zbigniew Jędrzejewski-Szmek
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-13 15:30 EDT by Michele Baldessari
Modified: 2016-02-09 15:53 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-02-09 15:53:40 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zbyszek: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Michele Baldessari 2015-10-13 15:30:42 EDT
Spec URL: http://acksyn.org/files/rpms/q-data-as-text/q-text-as-data.spec
SRPM URL: http://acksyn.org/files/rpms/q-data-as-text/q-text-as-data-1.5.0-1.fc22.src.rpm

Description:
q-text-as-data is a command line tool that allows direct execution of SQL-like
queries on CSVs/TSVs (and any other tabular text files).
 
q-text-as-data treats ordinary files as database tables, and supports all SQL
constructs, such as WHERE, GROUP BY, JOINs etc. It supports automatic column
name and column type detection, and provides full support for multiple
encodings.

Fedora Account System Username: mbaldessari
Comment 1 Zbigniew Jędrzejewski-Szmek 2016-01-10 15:56:45 EST
Summary should not repeat the name. Something like this instead:
Summary: Direct execution of SQL-like queries on tabular text files

Don't gzip the man page. It will be done automatically by rpm.
Also use q-test-as-data.1.* in %files (not .gz), so there is no need to adjust if the compression scheme ever changes.

Why 'rm -f'? The removal should not fail under any circumstances, so plain 'rm' should be used.

You have with_python3 defined, but it is not used anywhere.

Does the program support python3? If yes, then it should run under python3.

Independently of whether python2 or python3 is used, the shebang line should be changed from /usr/bin/env... to /usr/bin/python2 or /usr/bin/python3. Try:
sed -i '1s/#!.*/#!%{__python2}/' FILENAME
Comment 2 Michele Baldessari 2016-01-31 13:36:19 EST
Thanks Zbigniew!

(In reply to Zbigniew Jędrzejewski-Szmek from comment #1)
> Summary should not repeat the name. Something like this instead:
> Summary: Direct execution of SQL-like queries on tabular text files

ok, done

> Don't gzip the man page. It will be done automatically by rpm.
> Also use q-test-as-data.1.* in %files (not .gz), so there is no need to
> adjust if the compression scheme ever changes.

ok, done

> Why 'rm -f'? The removal should not fail under any circumstances, so plain
> 'rm' should be used.

right, done
 
> You have with_python3 defined, but it is not used anywhere.
> 
> Does the program support python3? If yes, then it should run under python3.
> 
> Independently of whether python2 or python3 is used, the shebang line should
> be changed from /usr/bin/env... to /usr/bin/python2 or /usr/bin/python3. Try:
> sed -i '1s/#!.*/#!%{__python2}/' FILENAME

So I have added python3 support in the package, so we just need to flip
the switch once python3 support lands (I made a note of which github issue
tracks this in the spec file).

Spec URL: http://acksyn.org/files/rpms/q-data-as-text/q-text-as-data.spec
SRPM URL: http://acksyn.org/files/rpms/q-data-as-text/q-text-as-data-1.5.0-2.fc23.src.rpm

Thanks again
Comment 3 Zbigniew Jędrzejewski-Szmek 2016-01-31 14:33:43 EST
Please note that this spec file will not build if with_pytohn3 is undefined (e.g. on RHEL). "%if %{?with_python3}" would expand to "%if", which is not valid. It works for Fedora though, so as far as this review goes, that's enough.

"%if 0%{?with_python3}" would make more sense, and then there's no need to define with_python3 to 0, just define it to 1 in the future.

+ license is acceptable (GPLv3)
+ license file is present, %license is used
+ provides and requires look OK
+ builds and installs fine
+ name is ok
+ latest version
+ no scriptlets present or necessary

Package is APPROVED.
Comment 4 Gwyn Ciesla 2016-01-31 16:37:19 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/q-text-as-data
Comment 5 Fedora Update System 2016-02-01 02:13:50 EST
q-text-as-data-1.5.0-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-0c4c01223d
Comment 6 Fedora Update System 2016-02-01 21:27:21 EST
q-text-as-data-1.5.0-2.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-0c4c01223d
Comment 7 Fedora Update System 2016-02-09 15:53:38 EST
q-text-as-data-1.5.0-2.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.