Bug 233715 - Review Request: rss2email - Deliver news from RSS feeds to your smtp server as text or html mail
Summary: Review Request: rss2email - Deliver news from RSS feeds to your smtp server a...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Patrice Dumas
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 233743
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-03-23 23:03 UTC by Thorsten Leemhuis
Modified: 2007-11-30 22:12 UTC (History)
1 user (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2007-03-26 08:18:35 UTC
Type: ---
Embargoed:
pertusus: fedora-review+
petersen: fedora-cvs+


Attachments (Terms of Use)

Description Thorsten Leemhuis 2007-03-23 23:03:05 UTC
Spec URL:
http://www.leemhuis.info/files/fedorarpms/SPECS.fdr/rss2email.spec
SRPM URL: 
http://www.leemhuis.info/files/fedorarpms/SRPMS.fdr/rss2email-2.60-1.src.rpm

Description: 
rss2email lets you subscribe to a list of XML newsfeeds (RSS or Atom). It can
parse them regularly with the help of cron and send new items to you by email.

A HTML mail will be send in the default configuration to the local smtp server.
See the man page r2e for details how to set rss2email up.

Comment 1 Patrice Dumas 2007-03-23 23:12:47 UTC
In my opinion html2text.py should be in a separate package. 
It is clearly usefull without rss2email.

Comment 2 Thorsten Leemhuis 2007-03-24 07:17:28 UTC
(In reply to comment #1)
> In my opinion html2text.py should be in a separate package. 
> It is clearly usefull without rss2email.

Well, I have thought about that as well. But each package more in our repos is
more work to do for mirrors, yum (downloading headers, depsolving), pungi and
other stuff. So I decided against it for now, as the package is only 34k in
total. But thinking about it again I'm not that sure myself. So what do other
people think?

Comment 3 Thorsten Leemhuis 2007-03-24 13:20:12 UTC
Okay, I've thought about it some more and split it of. pythong-html2text now in
Bug 233743

Spec URL:
http://www.leemhuis.info/files/fedorarpms/SPECS.fdr/rss2email.spec
SRPM URL: 
http://www.leemhuis.info/files/fedorarpms/SRPMS.fdr/rss2email-2.60-2.src.rpm

Comment 4 Patrice Dumas 2007-03-24 17:41:48 UTC
* there are 2 patches in the debian patcheset, one for html2txt and
  one for rss2email that may be relevant.

* Maybe you could ask the debian maintainer to avoid directly patching
  the sources, but instead put all the debian patches below debian/ 
  such that you can apply the debian patcheset and use the patches 
  and files directly.
 
* the debian template file seems to be better than yours, with a lot
  of comments, but you may disagree ;-)

* I have some remarks on rss2email-r2e:

I may be wrong, but it seems to me that [[ ]] is not an sh feature.
Also = in conditional is more portable than ==. I can make a patch
if you like.

* I suggest doing 
sed -e -i 's;/usr/share;%{_datadir}/g' 
on r2e.

* dos2unix issue

* suggestion:
  remove / after $RPM_BUILD_ROOT

* I also suggest that you get in touch with the debian packager
  to share your r2e script and your patch.

Comment 5 Patrice Dumas 2007-03-24 17:43:58 UTC
(In reply to comment #4)

> * I suggest doing 
> sed -e -i 's;/usr/share;%{_datadir}/g' 
> on r2e.

Better (but still unteted):
sed -e -i 's;/usr/share;%{_datadir};g'

Comment 6 Thorsten Leemhuis 2007-03-25 07:54:01 UTC
Patrice, thx for looking over this.

> there are 2 patches in the debian patcheset [...]
Applied one of them; the others are non-obvious to me ATM, and I prefer to go
without them for now.

> the debian template file seems to be better than yours, with a lot
>  of comments, but you may disagree ;-)
Yes, I want to avoid maintaining a copy of what is already documented and
maintenanced in another place. I also dislike to have many comments that
describe each option in config files, as that IMHO hurts badly when you have to
merge older and new configurations, because you end up merging lots of changed
comments over if you do it with diff/meld or similar tools.

> I have some remarks on rss2email-r2e [...]
Fixed

> I suggest doing sed [...] on r2e.
Done 

> dos2unix issue
Fixed (stupid me)

> remove / after $RPM_BUILD_ROOT
Done.

> I also suggest that you get in touch with the debian packager
>  to share your r2e script and your patch.
Contacting upstream and the debian maintainer is the plan after I have been
using it for some days first and see if everythings works as expected.

* Updated package:

Spec URL:
http://www.leemhuis.info/files/fedorarpms/SPECS.fdr/rss2email.spec

Spec diff URL:
http://www.leemhuis.info/files/fedorarpms/DIFFS.fdr/rss2email.spec

SRPM URL: 
http://www.leemhuis.info/files/fedorarpms/SRPMS.fdr/rss2email-2.60-3.src.rpm

Comment 7 Patrice Dumas 2007-03-25 18:03:57 UTC
I am not sure that
[ "${2}" ]
is portable either. I would have used

[ z"${2}" != z ]



* rpmlint is silent
* free software, no license, but only one file
* follow guidelines
* match upstream, for CHANGELOG there is this expected diff
-    last updated 6 months ago
+    last updated 7 months ago
c1f452fffcbc03ad1b2fab2f9c42b76a  rss2email.py
* %files section right

APPROVED


Comment 8 Thorsten Leemhuis 2007-03-26 05:48:42 UTC
thx for the review Patrice!

(In reply to comment #7)
> I am not sure that [...] is portable either.

I'll look into it when discussing it with the debian and upstream developers.
thx for the hint.

> APPROVED

New Package CVS Request
=======================
Package Name: rss2email
Short Description: Deliver news from RSS feeds to your smtp server as text or
html mail
Owners: fedora
Branches: FC-6 EL-4 EL-5
InitialCC:

Comment 9 Jens Petersen 2007-03-26 07:38:47 UTC
added

Comment 10 Thorsten Leemhuis 2007-03-26 08:18:35 UTC
built, thx everyone


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