Opened 10 years ago

Closed 10 years ago

#12869 closed enhancement (fixed)

The warn-links option shouldn't always trigger full doc compilation

Reported by: hivert Owned by: hivert
Priority: major Milestone: sage-5.1
Component: documentation Keywords: Broken links, warnings
Cc: Merged in: sage-5.1.beta0
Authors: Florent Hivert Reviewers: Andrey Novoseltsev
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12849 Stopgaps:

Status badges

Attachments (1)

trac_12869-nitpick_improve-fh.2.patch (2.5 KB) - added by novoselt 10 years ago.
Typo fixes

Download all attachments as: .zip

Change History (25)

comment:1 Changed 10 years ago by hivert

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 follow-up: Changed 10 years ago by novoselt

Does not apply on Sage-5.0.beta13...

comment:3 in reply to: ↑ 2 Changed 10 years ago by hivert

  • Dependencies set to #12849

Replying to novoselt:

Does not apply on Sage-5.0.beta13...

Sorry ! I forgot to tell it depend on #12849

comment:4 Changed 10 years ago by novoselt

Hmmm, where is "--warn-links" option described (except for #9128)? I don't see it in the command line help.

comment:5 Changed 10 years ago by hivert

Look carefully ;-)

$ sage -docbuild help
Usage: sage -docbuild [OPTIONS] DOCUMENT (FORMAT | COMMAND)

Build or return information about Sage documentation.
    DOCUMENT    name of the document to build
    FORMAT      document output format
    COMMAND     document-specific command
A DOCUMENT and either a FORMAT or a COMMAND are required,
unless a list of one or more of these is requested.

OPTIONS:
  Standard:
    [...]
    --warn-links        issue a warning whenever a link is not properly
                        resolved; equivalent to '--sphinx-opts -n' (sphinx
                        option: nitpicky)
    [...]

Florent

comment:6 follow-up: Changed 10 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev
  • Status changed from needs_review to positive_review

Aha, then my complain is: how should I know that "sage -docbuild help" works? It does not seem that it is visible in "sage -advanced" and it is not something common to other options.

Regarding the ticket itself - it work fine for me. I don't quite understand the details of the code, but from comments it is clear what it is supposed to do, tests pass. So positive review.

comment:7 in reply to: ↑ 6 Changed 10 years ago by hivert

Replying to novoselt:

Regarding the ticket itself - it work fine for me. I don't quite understand the details of the code, but from comments it is clear what it is supposed to do, tests pass. So positive review.

Here are some details:

In the file sphinx/environment.py, the class BuildEnvironment is responsible for handling the environment. It has a method called update which decide which files should be rebuild. Here is the relevant code:

class BuildEnvironment:
    [...]
    def update(self, config, srcdir, doctreedir, app=None):
        """(Re-)read all files new or changed since last update.

        Returns a summary, the total count of documents to reread and an
        iterator that yields docnames as it processes them.  Store all
        environment docnames in the canonical format (ie using SEP as a
        separator in place of os.path.sep).
        """
        config_changed = False
        if self.config is None:
            msg = '[new config] '
            config_changed = True
        else:
            # check if a config value was changed that affects how
            # doctrees are read
            for key, descr in config.values.iteritems():
                if descr[1] != 'env':
                    continue
                if self.config[key] != config[key]:
                    msg = '[config changed] '
                    config_changed = True
                    break
            [...]

So I change config.values['nitpick'][1] from 'env' to 'sage' to make sure that config changed is not set.

Florent

comment:8 Changed 10 years ago by hivert

I just spotted a miss-print in my patch

    ('py:class',' twisted.web2.resource.PostableResource'))

should be

    ('py:class', 'twisted.web2.resource.PostableResource')) 

Mind the quote before twisted. I'm re-uploading the patch an sete back to needs-review.

Sorry,

Florent

comment:9 Changed 10 years ago by hivert

  • Status changed from positive_review to needs_work

comment:10 Changed 10 years ago by hivert

  • Status changed from needs_work to needs_review

comment:11 follow-up: Changed 10 years ago by novoselt

How could it possibly work without the matching quote? (And I did check that it worked!) Will test the new patch in a few hours.

comment:12 in reply to: ↑ 11 Changed 10 years ago by hivert

Replying to novoselt:

How could it possibly work without the matching quote? (And I did check that it worked!) Will test the new patch in a few hours.

Did you really check that the broken links warning to twisted.* stuff where correctly silenced ?

Florent

comment:13 Changed 10 years ago by novoselt

I checked that documentation recompilation was not triggered by adding/removing "--warn-links" after this patch applied. I thought that if a module has mismatched quotes, it will not load correctly and since the patch touches a single file - obviously it got processed since it changed the rebuilding behaviour.

comment:14 Changed 10 years ago by jhpalmieri

(It looks like there was a matching quote; there was just an extra leading space before "twisted".)

comment:15 follow-up: Changed 10 years ago by novoselt

  • Status changed from needs_review to positive_review

Yeap, I was too concentrated on that leading space...

comment:16 in reply to: ↑ 15 Changed 10 years ago by hivert

Replying to novoselt:

Yeap, I was too concentrated on that leading space...

Thanks for the review... Next steps:

  • parallelisation of the doc build
  • table of contents for methods in a class.

comment:17 Changed 10 years ago by jdemeyer

  • Work issues set to typos

Just noticed a typo in the patch: "environement" should be "environment" (twice). I understand the mistake though... :-)

Changed 10 years ago by novoselt

Typo fixes

comment:18 follow-up: Changed 10 years ago by novoselt

  • Description modified (diff)
  • Work issues typos deleted

Fixed.

comment:19 in reply to: ↑ 18 Changed 10 years ago by hivert

Replying to novoselt:

Fixed.

Oups ! Pardon my French ;-)

Florent

comment:20 Changed 10 years ago by jdemeyer

  • Summary changed from The warn-links option shouldn't allways triggers full doc compilation. to The warn-links option shouldn't always trigger full doc compilation

comment:21 Changed 10 years ago by hivert

Hi Jeroen,

Could it be possible to have this one in 5.0 rather than 5.1, without it cleaning up the doc is really painful.

Florent

comment:22 follow-up: Changed 10 years ago by jdemeyer

Since this patch really only benefits developers, what's the difference if it gets merged in sage-5.0.rc0 or sage-5.1.beta0?

comment:23 in reply to: ↑ 22 Changed 10 years ago by hivert

Replying to jdemeyer:

Since this patch really only benefits developers, what's the difference if it gets merged in sage-5.0.rc0 or sage-5.1.beta0?

It depends on the time between the rc0 and the next beta0 release ;-) You are the boss on that. By the way, thanks a lot for your great job. We should say thanks more often !

Florent

comment:24 Changed 10 years ago by jdemeyer

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