Opened 9 years ago

Closed 9 years ago

#10351 closed defect (fixed)

Fix the way sage/misc/sagedoc.py reads doc/common/builder.py

Reported by: jdemeyer Owned by: mvngu
Priority: major Milestone: sage-4.6.2
Component: documentation Keywords:
Cc: Merged in: sage-4.6.2.alpha4
Authors: Jeroen Demeyer Reviewers: Mike Hansen, Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

The following code from sage/misc/sagedoc.py is horrible:

        # check if any documentation is missing.  first read the start
        # of SAGE_ROOT/devel/sage/doc/common/builder.py to find list
        # of languages, documents, and documents to omit
        doc_path = os.path.join(ROOT, 'devel', 'sage', 'doc')
        builder = open(os.path.join(doc_path, 'common', 'builder.py'))
        s = builder.read()[:1000]
        builder.close()
        # list of languages
        lang = []
        idx = s.find("LANGUAGES")
        if idx != -1:
            start = s[idx:].find('[')
            end =  s[idx:].find(']')
            if start != -1 and end != -1:
                lang = s[idx+start+1:idx+end].translate(None, "'\" ").split(",")
        # documents in SAGE_ROOT/devel/sage/doc/LANG/ to omit
        omit = []
        idx = s.find("OMIT")
        if idx != -1:
            start = s[idx:].find('[')
            end =  s[idx:].find(']')
            if start != -1 and end != -1:
                omit = s[idx+start+1:idx+end].translate(None, "'\" ").split(",")

Attachments (1)

10351_sagedoc.patch (5.6 KB) - added by jdemeyer 9 years ago.
SAGELIB patch

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by jdemeyer

  • Status changed from new to needs_review

comment:2 follow-up: Changed 9 years ago by mhansen

  • Reviewers set to Mike Hansen
  • Status changed from needs_review to needs_work

The method of saving the old path in this patch doesn't work since oldpath = sys.path has them both "pointing" to the same object. Then, when you change sys.path, you also change oldpath. You could change it to something like oldpath = sys.path[:]

comment:3 in reply to: ↑ 2 Changed 9 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Replying to mhansen:

The method of saving the old path in this patch doesn't work since oldpath = sys.path has them both "pointing" to the same object. Then, when you change sys.path, you also change oldpath. You could change it to something like oldpath = sys.path[:]

Thanks! I've solved the problem with

sys.path = oldpath + [os.path.join(ROOT, 'devel', 'sage', 'doc', 'common')]

comment:4 follow-up: Changed 9 years ago by leif

I wonder if the docbuilder "options" should go into a separate file, such that we don't (have to) import the whole builder.py.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by jdemeyer

Replying to leif:

I wonder if the docbuilder "options" should go into a separate file, such that we don't (have to) import the whole builder.py.

That's a possibility, but I think the current approach works fine (and builder.py is not such a big file).

comment:6 in reply to: ↑ 5 Changed 9 years ago by leif

Replying to jdemeyer:

Replying to leif:

I wonder if the docbuilder "options" should go into a separate file, such that we don't (have to) import the whole builder.py.

That's a possibility, but I think the current approach works fine (and builder.py is not such a big file).

Well, importing the whole file could have very strange side-effects, at least in the future.

Another reason is users are likely to edit (just) these options. (Which however is sub-optimal by itself...)

comment:7 Changed 9 years ago by jdemeyer

I can see your point, maybe it would not be a bad idea to split the configuration from the code...

comment:8 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:9 Changed 9 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Changed 9 years ago by jdemeyer

SAGELIB patch

comment:10 Changed 9 years ago by vbraun

  • Reviewers changed from Mike Hansen to Mike Hansen, Volker Braun
  • Status changed from needs_review to positive_review

Definite improvement ;-)

comment:11 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.