Bug 454921 - Review Request: python-dotconf - Parser for dot.conf file
Review Request: python-dotconf - Parser for dot.conf file
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-10 15:46 EDT by Hemant Goyal
Modified: 2008-08-01 05:39 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-01 05:39:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hemant Goyal 2008-07-10 15:46:15 EDT
Spec URL: http://www.nsitonline.in/hemant/stuff/pydotconf/rpm/pydotconf.spec
SRPM URL: http://www.nsitonline.in/hemant/stuff/pydotconf/rpm/pydotconf-0.1-1.src.rpm
Description: python parser for dot.conf file
Comment 1 Kyle VanderBeek 2008-07-10 16:07:52 EDT
At a glance you're going to need to change your spec quite a bit.  Please refer
to the packaging guidelines:

https://fedoraproject.org/wiki/Packaging/Guidelines
https://fedoraproject.org/wiki/Packaging/Python

In particular, don't include a Vendor, don't define things like name and version
needlessly, Source0 should be an URL, URL should be the package's home page, and
the package needs to be named python-pydotconf.

(Not to mention that putting "py" in the name of your python library is a bit
silly.  Why not just name it dotconf?  Also, you should consider reading PEP-8
and just rolling everything into a single dotconf.py instead of a directory...
it's only about 350 lines.)

You should also install the rpmdevtools RPM and use the "rpmdev-newspec -t
python" to generate a python-esque base .spec file.
Comment 2 Hemant Goyal 2008-07-11 15:24:31 EDT
Hi Kyle,

Thanks for the review.

SPEC URL :
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf.spec

SRPM URL:
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf-0.2-2.fc7.src.rpm

> In particular, don't include a Vendor, don't define things like name and
version, needlessly, Source0 should be an URL, URL should be the package's home
page, and the package needs to be named python-pydotconf.

Yes this was my mistake, I directly worked on the SPEC generated by disutils
bdist command. I have use the rpmdev (python) SPEC template now.

> (Not to mention that putting "py" in the name of your python library is a bit
> silly.  Why not just name it dotconf?  Also, you should consider reading PEP-8
> and just rolling everything into a single dotconf.py instead of a directory...
> it's only about 350 lines.)

Hmmm, thanks for the idea. I ve merged the two files into dotconf.py now. Are
you also suggesting that I do not include the file in a directory like dotconf?

Thanks,
Hemant
Comment 5 Mamoru TASAKA 2008-07-17 10:29:52 EDT
Would you review my review request (bug 453944) instread?
(note that scratch builds posted on the bug are now out of date)
Comment 6 Mamoru TASAKA 2008-07-17 10:42:14 EDT
Currently dist-f10 python is completely broken so please wait...
Comment 7 Mamoru TASAKA 2008-07-17 12:16:01 EDT
BTW

* Your latest package does not build on dist-f9-updates-candidate:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=722566
  egg info is created on F-9/devel (I forgot for F-8)

* Currently all branches are >= F-8 and %if 0%{?fedora} >= 8 is of no sense
  (unless you are thinking of OLPC branch)

* Currently no issue (as they are same), however would you clarify 
  this package is GPLv3 or GPLv3+?
Comment 8 Hemant Goyal 2008-07-18 04:57:06 EDT
Hi Mamoru,

Thanks for the review.

Package Updated.

SPEC URL :
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf.spec
 
SRPM URL:
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf-0.2-5.fc7.src.rpm


(In reply to comment #7)
> BTW
> 
> * Your latest package does not build on dist-f9-updates-candidate:
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=722566
>   egg info is created on F-9/devel (I forgot for F-8)

It just did not strike me to scratch build on koji myself.

dist-f9
http://koji.fedoraproject.org/koji/taskinfo?taskID=723978 

dist-f10
http://koji.fedoraproject.org/koji/taskinfo?taskID=723983

dist-f8
http://koji.fedoraproject.org/koji/taskinfo?taskID=723982

dist-olpc3
http://koji.fedoraproject.org/koji/taskinfo?taskID=723985

> * Currently all branches are >= F-8 and %if 0%{?fedora} >= 8 is of no sense
>   (unless you are thinking of OLPC branch)

Yes I will be requesting an OLPC branch. However I have fixed it as follows 

%if 1%{?olpc}
BuildRequires: python-setuptools-devel
%else
BuildRequires: python-setuptools
%endif   

> * Currently no issue (as they are same), however would you clarify 
>   this package is GPLv3 or GPLv3+?

Thanks for checking this up. The license should have been GPLv3+. I have fixed
this now, however it will not be reflected in the SRPM built on koji scratch
build. I have made the requisite fix in the SPEC that that I have uploaded.

Thanks
Comment 9 Mamoru TASAKA 2008-07-18 09:10:13 EDT
Well,

* Duplicate files
-----------------------------------------------------------------------
%dir %{python_sitelib}/dotconf
%{python_sitelib}/*
-----------------------------------------------------------------------
  Now the directory %{python_sitelib}/dotconf is listed twice.

* %if 1%{?olpc}
  This is always true.
Comment 10 Kyle VanderBeek 2008-07-18 16:16:14 EDT
(In reply to comment #2)
> Hmmm, thanks for the idea. I ve merged the two files into dotconf.py now. Are
> you also suggesting that I do not include the file in a directory like dotconf?

Yes, at this point there seems little reason to have a package directory since
it's just one file.  Move all the code into a dotconf.py file and out of the
subdirectory, delete and use the "py_modules" parameter in your setup.py:

  http://docs.python.org/dist/listing-modules.html

Additionally, please start creating tags in your google code svn repository. 
We'd like to see things a little more professionally versioned.
Comment 11 Hemant Goyal 2008-07-20 08:53:01 EDT
Thanks for the review.

Package Updated.

SPEC URL :
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf.spec
 
SRPM URL:
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf-0.2-6.fc7.src.rpm

@Kyle : Great I did not know about the concept of tagging until now :). I've
created the tags in the google SVN now. I've also moved dotconf.py out of the
directory and used py_modules parameter.

Thanks.
Comment 12 Mamoru TASAKA 2008-07-20 12:12:27 EDT
Did you change the tarball in the srpm without changing version?
(As you seem to be the upstream) please change the version and release the new one
formally if you want to change the tarball itself.
Comment 13 Hemant Goyal 2008-07-23 07:08:55 EDT
Hi,
I have changed the version of the tarball.

SPEC URL :
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf.spec
 
SRPM URL:
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf-0.2.1-7.fc7.src.rpm

Thanks,
Hemant
Comment 14 Mamoru TASAKA 2008-07-24 01:11:32 EDT
----------------------------------------------------------------------
    This package (python-dotconf) is APPROVED by me
----------------------------------------------------------------------
Comment 15 Hemant Goyal 2008-07-24 03:04:43 EDT
Thanks Mamoru :)

New Package CVS Request
=======================
Package Name: python-dotconf
Short Description: python parser for dot.conf file
Owners: hemantg
Branches: OLPC-2 OLPC-3 F-8 F-9
InitialCC:
Cvsextras Commits: yes
Comment 16 Kevin Fenzi 2008-07-24 14:12:16 EDT
cvs done.
Comment 17 Hemant Goyal 2008-08-01 05:39:31 EDT
Thank you for the CVS creation and the review.


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