Spec URL: http://jamielinux.fedorapeople.org/ledger/vim-ledger.spec SRPM URL: http://jamielinux.fedorapeople.org/ledger/SRPMS/vim-ledger-0-0.1.20131109git38d7a037.fc21.src.rpm Fedora Account System Username: jamielinux Description: Vim plugin for use with Ledger, the double-entry accounting system.
Hi This is my informal review. I comment about the spec file etc. SPEC: 1. %global commit 38d7a037 Referring to the following. https://fedoraproject.org/wiki/Packaging:SourceURL#Github 2. Group: Applications/Editors Remove them, no need to keep them now. 3. # Source0 is generated by running Source10, which pulls from the upstream # version control repository. Source0: %{name}-%{checkout}.tar.bz2 Source10: get-sources.sh # Include a copy of the license to comply with license requirements. # https://github.com/ledger/vim-ledger/pull/17 Source20: COPYING URL is necessary for the Source tag. https://fedoraproject.org/wiki/Packaging:SourceURL#Github Please remove source10 and source20. 4. Requires: vim-common Requires(post): vim Requires(postun): vim Please add comments on explicit dependencies. 5. %description %{summary}. Please write a detailed content in description. 6. %prep %setup -q -n %{name}-%{checkout} cp -p %{SOURCE20} . Why is "cp -p %{SOURCE20}" necessary here? 7. %postun rm %{installdir}/doc/tags Please remove "rm %{installdir}/doc/tags". It is dangerous rm in %postun.
Just wondering - shouldn't this "Require: ledger" also?
(In reply to NIWA Hideyuki from comment #1) > Hi > This is my informal review. I comment about the spec file etc. Thanks! :) > 1. %global commit 38d7a037 > > Referring to the following. > https://fedoraproject.org/wiki/Packaging:SourceURL#Github Fixed. > 2. Group: Applications/Editors > > Remove them, no need to keep them now. They are optional for Fedora, but kept for EPEL: https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag > 3. > URL is necessary for the Source tag. > https://fedoraproject.org/wiki/Packaging:SourceURL#Github > > Please remove source10 and source20. Fixed. > 4. Requires: vim-common > Requires(post): vim > Requires(postun): vim > > Please add comments on explicit dependencies. Added. > 5. %description > %{summary}. > > Please write a detailed content in description. Done. > 6. %prep > %setup -q -n %{name}-%{checkout} > cp -p %{SOURCE20} . > > Why is "cp -p %{SOURCE20}" necessary here? Removed. > 7. %postun > rm %{installdir}/doc/tags > > Please remove "rm %{installdir}/doc/tags". > It is dangerous rm in %postun. Removed. Spec URL: http://jamielinux.fedorapeople.org/ledger/vim-ledger.spec SRPM URL: http://jamielinux.fedorapeople.org/ledger/SRPMS/vim-ledger-0-0.2.20140504git06c6873.fc21.src.rpm * Mon Jul 14 2014 Jamie Nguyen <jamielinux> - 0-0.2.20140504git06c6873f - update to latest commit - use correct GitHub URL - fix explicit Requires - comments for explicit Requires - amend scriptlet commands - extend %%description
(In reply to Ankur Sinha (FranciscoD) from comment #2) > Just wondering - shouldn't this "Require: ledger" also? To view ledger files using Vim (with this plugin), you don't necessarily need to have ledger installed.
Oo, Jamie, any chance you're still interested in this after all these years? XD
No response---closing this.