Skip to content
This repository has been archived by the owner on Jun 18, 2020. It is now read-only.

Updated built-in sphinxify routine to support proposed changes in Sage #416

Merged
merged 2 commits into from Jan 24, 2018

Conversation

embray
Copy link
Contributor

@embray embray commented Dec 21, 2016

This is a change needed in order to support changes I'm proposing to Sage in https://trac.sagemath.org/ticket/21732

Those changes haven't been approved yet though, so obviously this PR should not be merged until something is finalized in that Trac ticket. Ideally, sagenb should probably not rely on Sage, or SAGE_DOC_SRC/SAGE_SRC at all for this, and I'd be in favor of a more extensive PR for sagenb to just include its own Sphinx configuration files for this purpose. But for now this is the simplest solution.

This is a change needed in order to support changes I'm proposing to Sage in https://trac.sagemath.org/ticket/21732

Those changes haven't been approved yet though, so obviously this PR should not be merged until something is finalized in that Trac ticket.  Ideally, sagenb should probably not rely on Sage at all for this, and I'd be in favor of a more extensive PR for sagenb to just include its own Sphinx configuration files for this purpose.  But for now this is the simplest solution.
@kcrisman
Copy link
Member

Well, ideally sagenb would be merged back into Sage proper. This seems fine otherwise. I suppose you could import SAGE_SRC only if necessary but probably it doesn't matter.

@embray
Copy link
Contributor Author

embray commented Dec 21, 2016

Well, ideally sagenb would be merged back into Sage proper

I disagree that that is ideal, but I guess this is not the place to discuss that.

In my Sage branch it's also possible for `SAGE_DOC_SRC` to be `None`, so that possibility has to be accounted for.  Again, none of this is set in stone yet
@jdemeyer
Copy link
Contributor

jdemeyer commented Mar 2, 2017

sphinxify has been move to Sage now, see src/sage/misc/sphinxify.py, why not use that instead and fix it in Sage?

@embray
Copy link
Contributor Author

embray commented Mar 3, 2017

IIRC in the ticket I linked to in the PR description I did fix it there. This PR was to apply the same fix to the copy here. I think there were reasons there was a separate copy being maintained in sagenb (perhaps for backwards-compatibility with older Sage versions).

@jdemeyer
Copy link
Contributor

jdemeyer commented Mar 3, 2017

I think there were reasons there was a separate copy being maintained in sagenb

I don't think that there are any such reasons, we don't expect newer versions of SageNB to work with older versions of Sage.

@embray
Copy link
Contributor Author

embray commented Mar 3, 2017

Maybe what I was actually thinking of was all this stuff:

def generate_configuration(directory):

@jdemeyer
Copy link
Contributor

jdemeyer commented Jan 24, 2018

If you read the PR description again, you will notice that merging this was a very bad idea.

This should be reverted (unless somebody has a very good reason why it should be merged anyway).

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

Successfully merging this pull request may close these issues.

None yet

4 participants