Skip to content
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

needimport path is not consistent to other directives #351

Closed
twodrops opened this issue Aug 4, 2021 · 4 comments
Closed

needimport path is not consistent to other directives #351

twodrops opened this issue Aug 4, 2021 · 4 comments
Assignees
Milestone

Comments

@twodrops
Copy link
Collaborator

twodrops commented Aug 4, 2021

Other directives like image or include use relative path starting from rSt file. Looks like this is the standard.
needimport uses relative path starting from folder where conf.py is. This behavior is not consistent with the other directives.

Hint: Change to this behavior will break existing builds. Possibly it shall be changed asap or never.

@danwos
Copy link
Member

danwos commented Aug 5, 2021

Good point.
Absolute paths are starting from conf.py folder for e.g. image directive.
Relative paths from rst file.

I'm totally open to change this behavior for needimport to the sphinx default way.
Will do it already with the upcoming release.

@danwos danwos added this to the 0.7.2 milestone Aug 5, 2021
@danwos
Copy link
Member

danwos commented Aug 6, 2021

This change will break the current implementation.

Maybe we can check if the "old" way is used to define the location (checking if file can be found) .
Then we throw an deprecation warning but go on without any error.

If old way does not lead to an existing file, use the new way (No warning).

@Chriner
Copy link

Chriner commented Aug 6, 2021

Wouldn't it be better to use the new way first?

  • If file is found at new position, then OK.
  • If not found at new position, then use old way
    • if file at old position found throw an deprecation warning and go on
    • if no file found throw error

@danwos danwos added this to To do in To-Do Priorities Aug 17, 2021
@danwos danwos moved this from Prio 3 to Prio 2 in To-Do Priorities Aug 17, 2021
@danwos
Copy link
Member

danwos commented Aug 18, 2021

@Chriner : You are right, that makes more sense.

For clarification:
conf.py under /home/me/project/docs.
Example rst under /home/me/project/docs/subfolder/page.rst

.. needimport:: /my.json   
    --> absolute from conf.py because of starting /
   --> /home/me/project/docs/my.json

.. needimport:: my.json  
   --> relative from current rst-file because of missing /
   --> /home/me/project/docs/subfolder/my.json

   * if not found, check /home/me/project/docs/my.json
     * if found there, use it but throw  DEPRECATION warning
     * if not found, raise already implemented exception/warning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

4 participants