Code review comment for lp:~adiroiban/pydoctor/1287458-objects.inv

Adi Roiban <email address hidden> writes:

>>> Also file is hardcoded to objects.inv and there is no option to use a different file name.
>>
>> That's also the way sphinx works right? It assumes that there is a
>> objects.inv at $base_url/objects.inv? If so, I agree there is no point
>> adding flexibility to pydoctor...
>
> This is the default... not sure why Sphinx configuration allows a custom file name.. maybe for extra flexibility.
> I think that we can start with simple default and this can be later extended.

+1

>
>>> def error(msg, *args):
>>> @@ -331,6 +332,8 @@
>>> 'warning',
>>> 'WARNING: guessing '+name+' for project name', thresh=-1)
>>> system.guessedprojectname = name
>>> + else:
>>> + system.guessedprojectname = system.options.projectname
>>
>> So this seems a bit strange ... I guess there should be a
>> system.projectname that gets set here and that should be referred to in
>> the places that currently check options.projectname and fall back to
>> guessedprojectname. I can do this if you don't want to though :-)
>
> OK. I have refactored this to use system.projectname.
>
> I am not sure why we still have projectname and guessedprojectname.

Thanks.

> Please check that I have not done anything stupid.

It looks OK.

>>> + domainname = 'obj'
>>> + base_url = full_name
>>> + self.msg('sphinx', "Unknown type for %s." % (full_name,))
>>
>> This seems like it would be cleaner to do a mapping based on obj.kind I
>> think?
>>
> Done!

Argh, so this was probably a bad suggestion, because .kind is actually
quite varied -- it can get set to, for example, "Instance Variable" or
"Class Method". So what had before was probably better.

But, to rescue myself a bit, you only need to do this to determine the
"domain". I think you can construct the link part by examining
obj.documentation_location -- see taglink in templatewriter/utils.py
(which could/should probably have the link computation part extracted --
I don't know why the 'link' function in there is like that, it's
misleadingly named :(). This also makes me realize that objects.inv
generation should take obj.isVisible into account...

Sorry for not mentioning this stuff earlier -- do you want to make these
changes? I can probably do it too, now the brain cells are warmed up a
bit.

Cheers,
mwh