Bug#688437: RFS: loook/0.6.8-1 ( ITP )

[Jakub Wilk <jwilk@debian.org>]

(I don't intend to sponsor this package.)

* Mechtilde Stehmann <ooo@mechtilde.de>, 2012-09-22, 18:43:
> http://mentors.debian.net/debian/pool/main/l/loook/loook_0.6.8-1.dsc

Lintian reports:

I: loook source: quilt-patch-missing-description loook-python3.patch
W: loook source: debian-rules-missing-recommended-target build-arch
W: loook source: debian-rules-missing-recommended-target build-indep
I: loook source: debian-watch-file-is-missing
P: loook: no-upstream-changelog
W: loook: unknown-section office

README.Debian would be good place to mention how the Debian package 
diverges from upstream. But Python 3.X _is_ supported by upstream, so I 
don't feel it's useful to mention it in README.Debian.

Have you actually read your README.source?

What is "<688397 is the bug number of your ITP>" supposed to mean? :)

I'd use "debhelper (>= 8)" instead of "debhelper (>= 8.0.0)".

Current standards version is 3.9.4.

If you don't use VCS, remove the commented-out Vcs-* fields.

It's conventional to put a space after each comma in Depends.

Please ask debian-l10n-english@lists.debian.org for review of the 
package description and the manual page.

Please add get-orig-source target to your debian/rules. Why was COPYING 
not included in the .orig.tar.gz?

What are build-stamp and configure-stamp targets for? They don't seem to 
do anything useful...

What is "-rm -Rf tmp" for?

The install target could be much simplified if you didn't compress the 
manpages manually, but let dh_compress do it for you. 

The package FTBFS when built twice in a row:

gzip -r9 /tmp/loook-0.6.8/debian/loook.man && \
mkdir -p debian/loook/usr/share/man/man1/
gzip: /tmp/loook-0.6.8/debian/loook.man: No such file or directory
make: *** [install] Error 1

I wouldn't use $(CURDIR) when it's not necessary; it makes build logs 
noisy for no good reason.

Now looking at upstream source:

> if os.getenv('USERPROFILE'):
>         config_path = os.getenv('USERPROFILE')
> elif os.getenv('HOME'):
>         config_path = os.getenv('HOME')

I recommend using: os.path.expanduser('~')

> elif os.name == 'dos':
>         config_path = "c:/"

This is dead code. DOS hasn't been supported since Python 2.3.

Application.saveConfig() saves the configuration always in UTF-8, but 
Application.__init__() would use locale encoding. Also, I think 
saveConfig() will fail badly if any of the settings contains special 
characters like # or %.

> if os.name == 'nt':
>         try:
>                 os.startfile(filename)
>         except:
>                 print("Warning: File could not be opened. - %s" % filename)
> else:
>         prg = self.ooo_path.get()
>         if not prg and os.name != 'nt':

The "os.name != 'nt'" check is redundant, it's always true.

> filename = filename.replace('"', '\\"')
> if os.name == 'dos':
>         filename = filename.replace('/', '\\')
>         prg = prg.replace('/', '\\')
> cmd = "\"%s\" \"%s\" &" % (prg, filename)
> if os.name == 'dos':
>         cmd = "\"%s\" \"%s\"" % (prg, filename)
> self.status.config(text="Starting viewer...")
> print(cmd)
> try:
>         res = os.system(cmd)
> except UnicodeError:
>         res = os.system(cmd)

This is not a correct way to escape filenames. Use subprocess.Popen to 
spawn external programs. Also, I have no idea was catching an exception 
and then re-running the very same code is supposed to achieve..

> def removeXMLMarkup(self, s, replace_with_space):
>         s = re.compile("<!--.*?-->", re.DOTALL).sub('', s)
>         repl = ''
>         if replace_with_space:
>                 repl = ' '
>         s = re.compile("<[^>]*>", re.DOTALL).sub(repl, s)
>         return s

This is not a correct way to parse XML. Use a real XML parser, e.g. 
xml.etree.

> parts = re.split("\s+", query.strip())

This could be rewritten more readably as:
parts = query.split()

> regex = re.compile(re.escape(query.lower()), re.DOTALL)
> if regex.search(docstring):
>         return 1

This is weird. Why re.DOTALL when there couldn't be any unescaped dot in 
the regular expression? Also why bother with compiling regular 
expression for a fixed string when it will be used only once? It could 
be more readable rewritten as:

if query.lower() in docstring:
         return 1

(There are more instances of very similar code in the same method...)

> # TODO: are all OOo files utf-8?

No, as far as I can tell there is no such guarantee.

> title_match = re.compile("<dc:title>(.*?)</dc:title>", re.DOTALL|re.IGNORECASE).search(docinfo)

Again, this is not a valid way to parse XML.

--
Jakub Wilk