Interwiki Module Fixed

Interwiki Module Fixed

[Thomas.J.Waldmann, 2013-06-21 12:36:26]

only a partial review, I must work on some other stuff now.

but you meanwhile can systematically fix the stuff I already found.

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/constants/keys.py
File MoinMoin/constants/keys.py (right):

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/constants/keys.py#...
MoinMoin/constants/keys.py:117: ALLOWED_FIELDS = [
constants.keys is a very global module, so "ALLOWED_KEYS" is not meaningful
enough as a name.
choose a better name (and maybe add a comment giving even more infos about what
it is about).

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/constants/keys.py#...
MoinMoin/constants/keys.py:120: ]
what usecase do you have for?:

DATAID

PARENTID

EXTERNALLINKS

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
File MoinMoin/util/_tests/test_interwiki.py (right):

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:31: 'OtherWiki/ns1/ns2':
'http://otherwiki.com/ns1/ns2/',
ok, so we are changing interwiki markup also
(as in: linking to other wikis)

this will need some change in the moin 1.9 importer to adapt the markup, so add
a # TODO about this.

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:46: (('SomePage', '', 'revid', '', revid,
'frontend.show_item', False), '/+show/+{0}/%40revid/SomePage'.format(revid)),
incorrect test, see below

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:49: (('SomePage', '', '', 'ns1', CURRENT,
'frontend.show_item', False), '/ns1/SomePage'),
seems like we had some extra-indication in the past.
/: :

vs

/ /

The question is: did that have some advantage / do we still need it?

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:50: (('SomePage', '', 'tags', 'ns1',
CURRENT, 'frontend.show_item', False), '/ns1/%40tags/SomePage'),
ehrm, really %40 ? when we (waldi an me) chose @ we checked that it is "safe"
for use in the path, so it should be "@" not "%40".

is your werkzeug/flask the latest version? if not, upgrade before doing anything
else.

also, this test doesn't look correct. You say @tags, but you are using
"SomePage" (which looks rather like a pagename, not a tag). so rather use
"sometag".

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:56: (('SomePage', '', 'revid', 'ns1/ns2',
revid, 'frontend.show_item_meta', False),
'/+meta/+{0}/ns1/ns2/%40revid/SomePage'.format(revid)),
that test is incorrect

your are using @revid so the "name" has to be the revid value.

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:123: (('http://example.org/',
u'SomePage', 'itemid', u'ns1'), 'http://example.org/ns1/@itemid/SomePage'),
test looks wrong - please systematically review all your changes and fix them.

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py
File MoinMoin/util/interwiki.py (right):

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:71: item_name = get_fqname(item_name, field,
namespace)
good, you removed some duplicated code :)

[Thomas.J.Waldmann, 2013-06-22 20:56:01]

some more comments

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
File MoinMoin/util/_tests/test_interwiki.py (right):

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:95: ('@tags/SomePage', ('Self', '',
'tags', 'SomePage')),
looks wrong, see above

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:98: ('OtherWiki/ns1/@chenid/Page',
('OtherWiki', 'ns1', '', '@chenid/Page')),
wut?

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:105:
('OtherWiki/ns1/ns2/@userid/OtherPage', ('OtherWiki', 'ns1/ns2', 'userid',
'OtherPage')),
looks wrong, see above

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:106:
('OtherWiki/ns3/ns2/@userid/OtherPage/foo', ('OtherWiki', '', '',
'ns3/ns2/@userid/OtherPage/foo')),
looks wrong, see above

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:134: tests = [('ns1/ns2/@itemid/Page',
('ns1/ns2', 'itemid', 'Page')),
looks wrong, see above

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:135: ('ns3/@itemid/Page', ('', '',
'ns3/@itemid/Page')),
looks wrong, see above

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:137: ('ns1/ns2/@tags', ('ns1/ns2',
'tags', '')),
sense?

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:138: ('@tags/cheenu', ('', 'tags',
'cheenu')),
wut?

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:139: ('ns1/ns2/@notid', ('ns1/ns2', '',
'@notid')),
i think we should disallow itemnames starting with @ (or +)

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/_tests/test_i...
MoinMoin/util/_tests/test_interwiki.py:140: ('ns1/ns2/ns3/@itemid/ns4',
('ns1/ns2', '', 'ns3/@itemid/ns4')),
sense?

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py
File MoinMoin/util/interwiki.py (right):

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:110: the namespaces are separated by colons (:).
fix docstring also

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:123: namespace_mapping.add(namespace.rstrip('/'))
why is this needed? and wasn't there before?
can be done in 1 line btw, using a set comprehension.

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:130: if token in namespace_mapping:
note: a set is not a mapping.

a mapping is something that maps something ("key") to something else ("value),
e.g. a dictionary.

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:144: return field in ALLOWED_FIELDS
it this necessary? seems a bit much to do a function call for just 1 simple
operation.

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:151: param: url: the composite name to split
btw, if you write new docstrings, please use rest markup (we use sphinx to
create api docs)

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:159: namespace, url =
_split_namespace(dict(app.cfg.namespace_mapping).keys(), url)
why the dict() creation?

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:164: field = tokens[0][1:]
duplicate operation

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:165: url = url[len(tokens[0])+1:]
if you do split('/', 1), you can directly get the rest

https://codereview.appspot.com/10439044/diff/3001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:182: 'MoinMoin/ns/@chenid/AnyPage' -> "MoinMoin",
"ns", "", "@chenid/AnyPage" if ns namespace exists and field chenid does not OR
wtf is chenid?

if you want some invalid field, say @invalidfield

[ashu1461, 2013-06-23 12:21:32]

https://codereview.appspot.com/10439044/diff/18001/MoinMoin/util/interwiki.py
File MoinMoin/util/interwiki.py (right):

https://codereview.appspot.com/10439044/diff/18001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:121: namespace_set = {namespace.rstrip('/') for
namespace in namespaces}
we need rstrip here, because the namespace name can be 
'ns1/', we had a test covering this.
earlier we did the same in _split_interwiki.

https://codereview.appspot.com/10439044/diff/18001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:150: namespace, url =
_split_namespace(dict(app.cfg.namespace_mapping).keys(), url)
the app.cfg.namespace_mapping appears to be list of tuples

[ReimarBauer, 2013-06-26 20:07:52]

some comments

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/constants/keys.py
File MoinMoin/constants/keys.py (right):

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/constants/keys.py...
MoinMoin/constants/keys.py:117: #Values that FIELD can take in the composite
name: [NAMESPACE/][@FIELD/]NAME
add a blank after a #

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/_tests/test_...
File MoinMoin/util/_tests/test_interwiki.py (right):

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/_tests/test_...
MoinMoin/util/_tests/test_interwiki.py:90: tests = [('', ('Self', '',
'name_exact', '')),
is name_exact not defined as a constant?

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py
File MoinMoin/util/interwiki.py (right):

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:124: namespace_set = {namespace.rstrip('/') for
namespace in namespaces}
what makes it a set type? or why postfix of _set

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:184: #TODO Now using slash('/') as delimiter in
interwiki markup instead of ':', change the moin 1.9 importer accordingly.
blank after #

[ashu1461, 2013-06-26 20:53:58]

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/constants/keys.py
File MoinMoin/constants/keys.py (right):

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/constants/keys.py...
MoinMoin/constants/keys.py:117: #Values that FIELD can take in the composite
name: [NAMESPACE/][@FIELD/]NAME
On 2013/06/26 20:07:52, ReimarBauer wrote:
> add a blank after a #

ok fine. :)

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/_tests/test_...
File MoinMoin/util/_tests/test_interwiki.py (right):

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/_tests/test_...
MoinMoin/util/_tests/test_interwiki.py:90: tests = [('', ('Self', '',
'name_exact', '')),
On 2013/06/26 20:07:52, ReimarBauer wrote:
> is name_exact not defined as a constant?
i was thinking about using keys here but
let us use u'name_exact' here only
because breaking of 'ns1/@name_exact/somename' into 'ns1', 'name_exact',
'somename' would be more clear here/

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py
File MoinMoin/util/interwiki.py (right):

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:124: namespace_set = {namespace.rstrip('/') for
namespace in namespaces}
On 2013/06/26 20:07:52, ReimarBauer wrote:
> what makes it a set type? or why postfix of _set

to make sure that we dont check on multiple values if namespaces = {'ns1/','ns1'
...}

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:184: #TODO Now using slash('/') as delimiter in
interwiki markup instead of ':', change the moin 1.9 importer accordingly.
On 2013/06/26 20:07:52, ReimarBauer wrote:
> blank after #

fine

[Thomas.J.Waldmann, 2013-06-27 12:19:08]

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py
File MoinMoin/util/interwiki.py (right):

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:124: namespace_set = {namespace.rstrip('/') for
namespace in namespaces}
from where does this get called with namespaces that have trailing slashes?

if there is some place, rather fix it THERE.

also, do not use names that have the data type appended, e.g. no "namespace_set"
or "foo_list". just use the plural for such cases, e.g. "namespaces".

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:144: Split a composite name into namespace, field and
pagename
your docstring reads a bit strange, considering that you GET a url and RETURN a
CompositeName.

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:154: namespace, url =
_split_namespace(dict(app.cfg.namespace_mapping).keys(), url)
why is dict() needed here?

https://codereview.appspot.com/10439044/diff/31001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:184: #TODO Now using slash('/') as delimiter in
interwiki markup instead of ':', change the moin 1.9 importer accordingly.
still not fixed. Review ALL the feedback you get AGAIN, fix ALL the stuff,
update codereview, do first codereview yourself (+ fix stuff you find) before
asking mentors for codereview.

[Thomas.J.Waldmann, 2013-06-27 20:11:24]

https://codereview.appspot.com/10439044/diff/47001/MoinMoin/util/interwiki.py
File MoinMoin/util/interwiki.py (right):

https://codereview.appspot.com/10439044/diff/47001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:159: Split a Fully qualified url into namespace,
field and pagename
fully

https://codereview.appspot.com/10439044/diff/47001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:174: if tokens[0] in FIELDS:
what happens if not?

[Thomas.J.Waldmann, 2013-06-27 21:17:35]

add a TODO to the moin19 converter that we need to support wikiname/itemname
syntax (convert from wikiname:itemname).

[Thomas.J.Waldmann, 2013-06-28 09:48:37]

https://codereview.appspot.com/10439044/diff/59001/MoinMoin/converter/moinwik...
File MoinMoin/converter/moinwiki19_in.py (right):

https://codereview.appspot.com/10439044/diff/59001/MoinMoin/converter/moinwik...
MoinMoin/converter/moinwiki19_in.py:28: # TODO Now using slash('/') as delimiter
in interwiki markup instead of ':', change the moin importer accordingly.
ehrm, this is the 1.9 converter (a converter that somehow behaves similar to
moin 1.9, e.g. support camelcase links etc.)

i doubt that we can easily support colons here instead of slashes (as you
changed all utility functions used for that), so that TODO should be moved to
the 1.9 importer code, which is MoinMoin/script/migration/moin19/... (or
alternatively: add a new ticket to the issue tracker).

https://codereview.appspot.com/10439044/diff/59001/MoinMoin/util/interwiki.py
File MoinMoin/util/interwiki.py (right):

https://codereview.appspot.com/10439044/diff/59001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:200: # TODO Now using slash('/') as delimiter in
interwiki markup instead of ':', change the moin 1.9 importer accordingly.
you already put this comment into the importer source code.
do you expect that someone will find and remove it here also after fixing the
importer code?

[Thomas.J.Waldmann, 2013-06-29 14:12:06]

ok

[waldi, 2013-06-29 14:30:10]

https://codereview.appspot.com/10439044/diff/68001/MoinMoin/util/interwiki.py
File MoinMoin/util/interwiki.py (right):

https://codereview.appspot.com/10439044/diff/68001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:43: def get_fqname(item_name, field, namespace):
You may want to convert this to a member of CompositeName later.

https://codereview.appspot.com/10439044/diff/68001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:50: if field:
"if field and field != NAME_EXACT:"
may be faster and easier to read.

https://codereview.appspot.com/10439044/diff/68001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:146: def split(self):
Is this used? This looks like backward compatibility code and defines a
different view over the same data. If it is used, document it, otherwise remove
it.

https://codereview.appspot.com/10439044/diff/68001/MoinMoin/util/interwiki.py...
MoinMoin/util/interwiki.py:157: def split_fqname(url):
You may want to convert this to a (static) member method of CompositeName later