Bug 1271379 - Review Request: q-text-as-data - Q - Text as Data
Summary: Review Request: q-text-as-data - Q - Text as Data
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-10-13 19:30 UTC by Michele Baldessari
Modified: 2016-02-09 20:53 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-02-09 20:53:40 UTC
Type: Bug
Embargoed:
zbyszek: fedora-review+


Attachments (Terms of Use)

Description Michele Baldessari 2015-10-13 19:30:42 UTC
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 20:56:45 UTC
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 18:36:19 UTC
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 19:33:43 UTC
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 21:37:19 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/q-text-as-data

Comment 5 Fedora Update System 2016-02-01 07:13:50 UTC
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-02 02:27:21 UTC
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 20:53:38 UTC
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.