samizdat-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: New snapshot, patches review


From: boud
Subject: Re: New snapshot, patches review
Date: Mon, 25 Sep 2006 02:08:09 +0200 (CEST)

hi again samizdat-devel,

On Sun, 24 Sep 2006, Dmitry Borodaenko wrote:

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

Right now, i have:
http://packages.debian.org/experimental/web/samizdat
Package: samizdat (0.5.5.20060914-1)

i'll try again later - i presume this is just part of mirroring or
normal debian checking mechanisms.


GETTEXT FIX:

It incorporates Boud's GetText fix (please review my version to see how
I think it should be done)

Your version looks fine to me, but the question which i still have (i
forgot to ask it) is that it seems to me unnecessary and bad in terms
of operating speed to check the gettext version every time someone
starts a session. From my (beginner's) experience in C, my instant
reaction is that this should be done by preprocessor #if and #define
commands, so that it only gets tested during compilation. On the other
hand, i only have a vague understanding of the full
compilation/interpretation cycle, so i'm not sure that this is actually
worth worrying about.


FR.PO, EMAIL:

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

Seems reasonable to me.


README.locale and RSS import hack.


README.locale:


README.locale:


(1) rgettext-1.15 (2004/11/05) (debian-sarge) has a bug.
(2) rmsgmerge does not seem to be part of the standard ruby distribution 1.8
(3) my error on [country-code]

It shouldn't be Debian-specific,

Sure.

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.

OK as a matter of principle.


(1) rgettext-1.15 (2004/11/05) (debian-sarge) has a bug, and using it requires a hack.


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

rgettext-1.15 eems to be buggy :( - it misses some expressions e.g. the
ratings in template.rb:

prompt$ rgettext --version
/usr/bin/rgettext 1.15 (2004/11/05)


prompt$ grep "_('-\?[012]" lib/samizdat/engine/template.rb
            [-2, _('-2 (No)')],
            [-1, _('-1 (Not Likely)')],
            [0, _('0 (Uncertain)')],
            [1, _('1 (Likely)')],
            [2, _('2 (Yes)')] ], 0]

prompt$  rgettext lib/samizdat/engine/template.rb  | grep "_('-\?[012]" |wc
      0       0       0


hypothesis: i think the reason is nested { { } } - as in the following line in template.rb:

    link ? %{<div class="nav"><a href="#{link}">rss 1.0</a></div>} : ''

If you add in a dummy expression e.g.

## HACK TEST ONLY # value = _('Likely')
## END OF HACK

following this line and then remove or protect \{ \} the parentheses,
you'll find a difference with whether or not rgettext-1.15 picks up
'Likely' or not.


A hack that works for me is:

sed -e 's,\#{\([^}]*\)},\#\\\{\1\\\},g' \
 cgi-bin/*.rb lib/samizdat/*.rb lib/samizdat/engine/*.rb > .tmp && \
 rgettext .tmp -o po/samizdat.pot

(pipe | didn't work because rgettext doesn't seem to like stdin)


i tried looking for a more recent version of rgettext, but i found this:

Ruby-GetText-Package-1.8.0  2006-09-11 19:24
http://rubyforge.org/frs/?group_id=855

which by its version number is much older than rgettext 1.15, though
by its date is about but two years more recent, so i presume that
Ruby-GetText-Package and rgettext have different numbering schemes.

Also, Ruby-GetText-Package-1.8.0 says it requires ruby 1.8.3 or more recent,
whereas debian-sarge has 1.8.2, and IMHO we should be aiming at samizdat
requiring as few as possible packages more recent than debian stable.

i'm preparing a new version of README.locale, but i'm interested what
people think of the  rgettext bug and whether or not we should report
it to the Ruby-GetText-Package people (someone would need to test it
with a more recent version than rgettext --version = 1.15).


(2) rmsgmerge is not in my debian-sarge ruby distribution

and then, if you want to update existing translaton:

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

However, msgmerge is available and seems to do the job OK.


(3) my error on [country-code]

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.

Yes, of course. /me was tired.



RSS IMPORT HACK:

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.

Sure, sorry if i was unclear on that - i thought it was clear from my
lack of experience in ruby and samizdat that this was a real "hack" in the sense of something-that-will-work-now-in-one-situation and
probably a long way from a systematic, well-designed solution.


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

i agree.

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

OK, i'll find out how to use open-uri.

+  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 :)

Sure.

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

Actually this is something i've been wondering about. Certainly having it
in config.yaml (or rather sites.yaml ?) would be better than hardwired
into index.rb ;).

But it also seems to me a reasonable idea that moderators, after facilitating decisions on adding/removing feeds, should be able to add/remove
them without server admin intervention.

In other words, surely it should be a "resource" like any other resource?
i'm still rather fuzzy about the different resource possibilities in samizdat, but i don't think it should be a "member" ;).

It seems to me that open publishing of RDF feeds is probably reasonable,
if feeds on the front page require a decision implemented by a moderator,
and feeds on a registered member's page just require that registered member to agree to it. So maybe it could be a message of uri-list format?

In any case, i think that what is present in mir-1.1 at the moment is that admin intervention is needed, so making this a partially open function
is probably not urgent.


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

I don't see why these 3 lines are necessary, when Ruby provides powerful
string interpolation.

Ummm... i know that ruby has lots of powerful string "methods" - but i'm
not sure why these three lines are not useful.

Not to mention that \"" is plain ugly. Use single
quotes, or %{} when you need double quotes inside a string.

OK

+    for feed_number in 0...import_feeds_list.length

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

OK.

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

OK (In fact it is based on a recommendation in the RTFM - the main web
page for the package...)


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

i thought i tried \s and it failed, but i can test it again...

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

That's a nice idea. i was thinking of, in the indymedia context,
having at least 3 different classes of feeds:

* local (e.g. other alternative media sites in the local city/region)
* regional (e.g. indymedia sites in central/eastern europe)
* global (like on the www.indymedia.org right-hand column)

and i was wondering where to put them. The left-hand column just might
possibly be a good place.

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.

Of course, this was just a first hack, which is why i didn't change more
than one css.

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.

Yes, i was thinking that it would be wasteful to continually pull the
feeds, so some sort of caching makes sense. (In fact, in testing this,
i had a problem with getting old cached copies of the html page - stopping both /etc/init.d/samizdat and apache was sufficient to get
rid of this problem.)

So maybe my hack was not as far off from a real solution as i thought
- based on your feedback and a few more iterations it might get to the
acceptable stage. :) On my next iteration, i'll try admin-level
/etc/samizdat definition of feeds, probably with 3 (or N) sub-classes
and sub-class names (default: local, regional, global) to go in the
left-hand column; also these could be created as individual pages,
linked from the front page, like "Moderation Log". (i think the idea
of more open-published type rdf feeds is possibly something to add to the ToDo list for sometime in the future, rather than right now.)

cheers
boud




reply via email to

[Prev in Thread] Current Thread [Next in Thread]