Bug 1094013

Summary:	Review Request: vim-ledger - Vim plugin for use with Ledger, the double-entry accounting system
Product:	[Fedora] Fedora	Reporter:	Jamie Nguyen <jamielinux>
Component:	Package Review	Assignee:	Nobody's working on this, feel free to take it <nobody>
Status:	CLOSED WONTFIX	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	niwa.hideyuki, package-review, sanjay.ankur
Target Milestone:	---
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2019-09-22 17:08:34 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:
Bug Depends On:
Bug Blocks:	201449

Description Jamie Nguyen 2014-05-04 10:48:19 UTC

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.

Comment 1 NIWA Hideyuki 2014-07-08 01:14:54 UTC

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.

Comment 2 Ankur Sinha (FranciscoD) 2014-07-11 07:18:17 UTC

Just wondering - shouldn't this "Require: ledger" also?

Comment 3 Jamie Nguyen 2014-07-14 08:08:54 UTC

(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

Comment 4 Jamie Nguyen 2014-07-14 08:09:57 UTC

(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.

Comment 5 Ankur Sinha (FranciscoD) 2019-08-04 17:48:11 UTC

Oo, Jamie, any chance you're still interested in this after all these years? XD

Comment 6 Ankur Sinha (FranciscoD) 2019-09-22 17:08:34 UTC

No response---closing this.