Skip to content

Fix regression in #81 #93

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 18, 2017
Merged

Fix regression in #81 #93

merged 1 commit into from
Jun 18, 2017

Conversation

jschueller
Copy link
Contributor

@jschueller jschueller commented May 10, 2017

It seems #81 introduced some kind of bug:

I have the following rst file documenting MyClass object from ottemplate module:

User manual
===========

Reference
---------

.. currentmodule:: ottemplate

.. autosummary::
    :toctree: _generated/
    :template: class.rst_t

    MyClass

with class.rst_t:

{{ objname }}
{{ underline }}

.. currentmodule:: {{ module }}

.. autoclass:: {{ objname }}

   {% block methods %}
   .. automethod:: __init__
   {% endblock %}

results in the following error:

following exception was raised:
Traceback (most recent call last):
  File "/home/schueller/.local/lib/python3.5/site-packages/sphinx/ext/autodoc.py", line 551, in import_object
    __import__(self.modname)
ImportError: No module named 'ottemplate.ottemplate.MyClass'; 'ottemplate.ottemplate' is not a package

I use sphinx latest stable 1.5.5 with python 3.5.2.

What do you think @sirno ?

@jschueller jschueller force-pushed the patch-2 branch 2 times, most recently from c560652 to f784df6 Compare May 10, 2017 16:05
@sirno
Copy link
Contributor

sirno commented May 10, 2017

Hey,

could it be that the double use of .. currentmodule:: causes the issue? your template
already sets the module, but then again this should also fail when someone wants to
autodoc multiple classes in the same .rst file, which would be an issue.

idk, back then i just knew i needed to include that line and searched for some way to
put it there. if you want i can take a closer look tomorrow.

@jschueller
Copy link
Contributor Author

jschueller commented May 11, 2017

Hi, thanks for your quick reply

I tried to get rid of my currentmodule but it doesnt work either.

It seems it's more complicated than that ; MyClass lies in ottemplate/ottemplate.py but in ottemplate/__init__.py I do from .ottemplate import * so from ottemplate import MyClass is valid.
So, the code location (ottemplate.ottemplate.MyClass) is different from the import location (ottemplate.MyClass). Then your code actually hardcodes the code location whereas it should use the import location.

I think we have no easy way of doing that ; I propose to revert to avoid regressions in lots of python codebases, what do you think ?

It seems numpy#81 introduced some kind of bug:

I have the following rst file documenting MyClass object from ottemplate module:
```
.. currentmodule:: ottemplate

.. autosummary::
    :toctree: _generated/
    :template: class.rst_t

    MyClass
```

with class.rst_t:
```
{{ objname }}
{{ underline }}

.. currentmodule:: {{ module }}

.. autoclass:: {{ objname }}

   {% block methods %}
   .. automethod:: __init__
   {% endblock %}

```
results in the following error:
```
following exception was raised:
Traceback (most recent call last):
  File "/home/schueller/.local/lib/python3.5/site-packages/sphinx/ext/autodoc.py", line 551, in import_object
    __import__(self.modname)
ImportError: No module named 'ottemplate.ottemplate.MyClass'; 'ottemplate.ottemplate' is not a package

```

What do you think @sirno ?
@jschueller jschueller mentioned this pull request May 12, 2017
jschueller added a commit to jschueller/openturns that referenced this pull request May 12, 2017
Fixes circleci build waiting for the numpydoc fix:
numpy/numpydoc#93
@jschueller
Copy link
Contributor Author

ping @sirno

@sirno
Copy link
Contributor

sirno commented May 17, 2017

hey, sorry i was quite busy and am without computer the next couple of days.

however, it shouldnt be a problem that you packaged like this since ottemplate.ottemplate.MyClass should still be directly importable from the absolute path.

either way. i think reverting is a good plan. i can easily fix my own stuff by manually adding the line
and it will probably will cause more headaches for many people once there is a release.

so if possible somebody should merge this pr before the next release.

@jschueller
Copy link
Contributor Author

hello maintainers (@rgommers @pv, @stefanv ?), could you review this one ?

@jschueller
Copy link
Contributor Author

ping

@stefanv
Copy link
Contributor

stefanv commented Jun 11, 2017

This was merged by @rgommers. Also see gh-86.

@jschueller
Copy link
Contributor Author

jschueller commented Jun 11, 2017

@stefanv #86 was meant to fix a single test introduced after #81, this basically reverts #81 as it introduced a severe regression

@stefanv
Copy link
Contributor

stefanv commented Jun 11, 2017

@jschueller Understood; just tying together all relevant PRs.

@rgommers rgommers merged commit e11e10c into numpy:master Jun 18, 2017
@rgommers
Copy link
Member

Looks like this makes sense to revert, thanks @jschueller, all

@jschueller jschueller deleted the patch-2 branch June 18, 2017 09:02
@rgommers rgommers added this to the v0.7.0 milestone Jun 18, 2017
jschueller added a commit to jschueller/openturns that referenced this pull request Jun 28, 2017
Fixes circleci build waiting for the numpydoc fix:
numpy/numpydoc#93
jschueller added a commit to openturns/openturns that referenced this pull request Jul 13, 2017
Fixes circleci build waiting for the numpydoc fix:
numpy/numpydoc#93
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants