New snapshot, patches review

[Dmitry Borodaenko]

New CVS snapshot 2006-09-24 is on Savannah and in Debian/experimental.

It incorporates Boud's GetText fix (please review my version to see how
I think it should be done) and French translation, as well as most of
the other suggestions (email is now commented out by default: it's not
right to require something that user is not always able to set up), except
README.locale and RSS import hack.

README.locale:

It shouldn't be Debian-specific, and it's not proper to de-compile
binary files when the source code is available. Technical or not, users
should be encouraged to take advantage of source code availability, this
way they will understand better how it empowers them.

The right way to work with Samizdat translations would be to run:

rgettext cgi-bin/*.rb lib/samizdat/*.rb lib/samizdat/engine/*.rb \
  -o po/samizdat.pot

and then, if you want to update existing translaton:

cd po/
rmsgmerge locale.po samizdat.pot -o locale.pot

The reference to [country-code] is incorrect and confusing: locale code
is usually a language code, optionally augmented by country code and
charset (only when necessary, e.g. for european and Brazilian
Portuguese). For Belarusian, language code is "be" and country code is
"BY", Belarusian translation of samizdat goes to
.../be/LC_MESSAGES/samizdat.mo.

RSS import hack:

This patch is wrong in more than one way and need to be reworked before
I am able to merge it in.

First of all, this code does not belong in index.rb's main body, it
should be moved to a function, if not to a separate file (as we may want
to display syndicated feeds in other places in addition to the front
page).

> +  require 'net/http'

Instead of Net::HTTP, use 'open-uri' library introduced in Ruby 1.8, it
is much cleaner and simpler to use. Use of Net::HTTP in unit tests is a
legacy from Ruby 1.6 times, some day I will get rid of it.

> +  import_feeds_list = [
> +    ['pl.indymedia.org', '/pl/syndycation/plindy-features.rdf'],
> +    ['cia.bzzz.net', '/node/feed']
> +    ]

There is no justification for code like this in a language that has
hashes (associative arrays). And indentation is wrong, too :)

And of course it should go into config.yaml instead.

> +  rss_pre= "<li><a href=\""

I don't see why these 3 lines are necessary, when Ruby provides powerful
string interpolation. Not to mention that \"" is plain ugly. Use single
quotes, or %{} when you need double quotes inside a string.

> +    for feed_number in 0...import_feeds_list.length

This is a very un-Ruby loop. Use each or collect.

> +      begin
> +        rss = RSS::Parser.parse(response.body)
> +      rescue RSS::InvalidRSSError
> +        rss = RSS::Parser.parse(response.body, false)
> +      end

When you do something as obscure as this, at least add a comment
explaining what you try to do or why you do it this way.

> +            link.sub(/[ \n\t]*(http[^ \n\t]*)[ \n\t]*/,'\1')

This regexp could also benefit from some explanation. Or from using \s
for whitespace matching.

> +    <th>#{import_feeds_title}</th>

I think three columns is enough, if not too much. Think of all the
people who have small screens or use large fonts. I would rather fit it
into the left column, below focuses and links.

This would also save you from the trouble of changing CSS. Each time you
change default.css, update all other styles too, or you will break them.

And finally, access to remote data (this applies not only to HTTP
requests, but also to database queries) should be cached whenever
possible. In this case, I would first parse the feed into something
simple and universal (such as array of hashes), and then cache it
together with the time when it was retrieved, so that it can be rendered
differently in different contexts and refreshed after some time.

--
Dmitry Borodaenko