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
Add 'Builder.epilog' attribute #4354
Add 'Builder.epilog' attribute #4354
Conversation
11f7f2e
to
5f6147d
Compare
sphinx/application.py
Outdated
@@ -338,6 +338,17 @@ def build(self, force_all=False, filenames=None): | |||
(status, self._warncount))) | |||
else: | |||
logger.info(bold(__('build %s.') % status)) | |||
|
|||
if self.statuscode == 0 and self.builder.epilog: | |||
# TODO(stephenfin): Should this use logging or print? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like your thoughts on both of these items, @tk0miya 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use logging module to output messages, warnings and errors.
sphinx/builders/texinfo.py
Outdated
@@ -97,6 +98,12 @@ class TexinfoBuilder(Builder): | |||
""" | |||
name = 'texinfo' | |||
format = 'texinfo' | |||
epilog = 'The Texinfo files are in %(outdir)s.' | |||
if os.name == 'posix': | |||
epilog += ("Run 'make' in that directory to run these through " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a space (or rather a \n
) is missing at start of string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
146cb87
to
34de6aa
Compare
sphinx/application.py
Outdated
@@ -338,6 +338,17 @@ def build(self, force_all=False, filenames=None): | |||
(status, self._warncount))) | |||
else: | |||
logger.info(bold(__('build %s.') % status)) | |||
|
|||
if self.statuscode == 0 and self.builder.epilog: | |||
# TODO(stephenfin): Should this use logging or print? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use logging module to output messages, warnings and errors.
sphinx/application.py
Outdated
# TODO(stephenfin): Should this use logging or print? | ||
logger.info('') | ||
# TODO(stephenfin): Should this be printf-based or | ||
# .format-based? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either is fine. Personally, I preferred printf-based one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll stick with printf. None of the disadvantages I can see with printf
would affect us here.
@@ -338,6 +338,17 @@ def build(self, force_all=False, filenames=None): | |||
(status, self._warncount))) | |||
else: | |||
logger.info(bold(__('build %s.') % status)) | |||
|
|||
if self.statuscode == 0 and self.builder.epilog: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving this to the end of Builder.build()
? I think emitting epilog message is responsibility of builder itself.
In addition, I'm worried about epilog
is better way or not. I think a method of builder is also good like Builder.show_finish_message()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving this to the end of
Builder.build()
? I think emitting epilog message is responsibility of builder itself.
I can do this. The reason I kept it separate was I wasn't sure if Builder.build()
was ever overridden by subclasses. If it was, we could end up emitting the message before any subclasses were called. For example:
class MySpecialBuilder(Builder):
...
def build(self, docnames, summary=None, method='update'):
super(MySpecialBuilder, self).build(docnames, summary, method)
# do something else here
It's your call though. Let me know what you'd prefer.
In addition, I'm worried about epilog is better way or not. I think a method of builder is also good like
Builder.show_finish_message()
.
I prototyped with both a function and a property but both seemed overkill. The logic is always the same: if the build succeeds then emit the message. Using printf
formatting will also ensure we get consistent builder messages. Is there any reason not to do things this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's your call though. Let me know what you'd prefer.
Actually, there was another reason. I wanted the summary message to come after the build succeeded
message. For example:
...
build succeeded
You can now process the pickle files in build/pickle.
I wasn't able to do this if I called this as part of Builder.finish()
. If we move this here, we should also move the generation of the build succeeded
message. I don't know if we want to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it was, we could end up emitting the message before any subclasses were called.
...
Actually, there was another reason. I wanted the summary message to come after the build succeeded message.
Reasonable. My idea was wrong. It should be shown on Builder.build()
.
Please forget this comment.
Is there any reason not to do things this way?
My worry is how to customize the epilog message on subclasses.
If they want to show more complicated message, an attribute is not extensible.
For example, it is hard to use out_suffix
to the message. On the other hand, a method is easy to override in subclasses.
I can't determine the interface is important or not. So I'd like to hear your idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason not to do things this way?
My worry is how to customize the epilog message on subclasses.
If they want to show more complicated message, an attribute is not extensible.
For example, it is hard to useout_suffix
to the message. On the other hand, a method is easy to override in subclasses.I can't determine the interface is important or not. So I'd like to hear your idea.
Sorry for the delay. The epilog
attribute covers all the provided builders. Also, if users really need to override this, then can use properties.
@property
def epilog(self):
...
Personally, I think this is adequate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough
34de6aa
to
08f5976
Compare
This allows builders to emit a final epilog message containing information such as where resulting files can be found. This is only emitted if the build was successful. This allows us to remove this content from the 'make_mode' tool and the legacy 'Makefile' and 'make.bat' templates. There's room for more dramatic simplification of the former, but this will come later. Signed-off-by: Stephen Finucane <stephen@that.guru>
These are handled by the default case. Signed-off-by: Stephen Finucane <stephen@that.guru>
Most of these are not necessary now that we're not printing different messages for various builders. Signed-off-by: Stephen Finucane <stephen@that.guru>
As with the Makefile previously, these are not necessary now that we're not printing anything different for various builders. Signed-off-by: Stephen Finucane <stephen@that.guru>
It's unlikely that anyone will need to do this but at least give them the opportunity. Signed-off-by: Stephen Finucane <stephen@that.guru>
It's unlikely that anyone will need to do this but at least give them the opportunity. Signed-off-by: Stephen Finucane <stephen@that.guru>
08f5976
to
b16fd2c
Compare
@@ -338,6 +338,17 @@ def build(self, force_all=False, filenames=None): | |||
(status, self._warncount))) | |||
else: | |||
logger.info(bold(__('build %s.') % status)) | |||
|
|||
if self.statuscode == 0 and self.builder.epilog: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough
if self.run_generic_build('html') > 0: | ||
return 1 | ||
print() | ||
print('Build finished. The HTML pages are in %s.' % self.builddir_join('html')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems "Build finished." message has been lost. Is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We already emit build succeeded
or build finished with problems
here. I thought this was duplicate information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I understand
BUILDDIR = {{ rbuilddir }} | ||
|
||
# Internal variables. | ||
PAPEROPT_a4 = -D latex_elements.papersize=a4 | ||
PAPEROPT_letter = -D latex_elements.papersize=letter | ||
ALLSPHINXOPTS = -d $(BUILDDIR)/doctrees $(PAPEROPT_$(PAPER)) $(SPHINXOPTS) {{ rsrcdir }} | ||
# $(O) is meant as a shortcut for $(SPHINXOPTS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know where $(O)
comes from? Is this special variable for GNU Make?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's just a shortcut added when make_mode was added. @jfbu noted that he uses this so I added it here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it shell variable like O=... make html
?
Okay, I almost understand. I don't know such shortcut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. It was added as part of make mode. It's just a usability helper.
@echo "Build finished. Dummy builder generates no files." | ||
# Catch-all target: route all unknown targets to Sphinx | ||
.PHONY: Makefile | ||
%: Makefile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this Makefile allows any argument, so this is like a Makefile for make-mode.
I think it's okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This will break if a user passes in an unknown builder, but Sphinx should show sufficient information. I can harden this if it would help (i.e. only allow one argument) but I don't know if that's necessary? Your call 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you show me the result of make foo bar
? Is sphinx-build invoked twice?
Anyway, make-mode is not used by default. So almost of users would not see this file. So either is fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you show me the result of make foo bar? Is sphinx-build invoked twice?
Sure. First, using valid builder names.
$ make html dirhtml
sphinx-build -b "html" -d build/doctrees source "build/html"
Running Sphinx v1.7+
loading pickled environment... done
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 0 source files that are out of date
updating environment: 0 added, 0 changed, 0 removed
looking for now-outdated files... none found
no targets are out of date.
build succeeded.
The HTML pages are in build/html.
sphinx-build -b "dirhtml" -d build/doctrees source "build/dirhtml"
Running Sphinx v1.7+
loading pickled environment... done
building [mo]: targets for 0 po files that are out of date
building [dirhtml]: targets for 0 source files that are out of date
updating environment: 0 added, 0 changed, 0 removed
looking for now-outdated files... none found
no targets are out of date.
build succeeded.
The HTML pages are in build/dirhtml.
Then using invalid builders.
$ make foo html
sphinx-build -b "foo" -d build/doctrees source "build/foo"
Running Sphinx v1.7+
Sphinx error:
Builder name foo not registered or available through entry point
make: *** [Makefile:96: foo] Error 2
Anyway, make-mode is not used by default. So almost of users would not see this file. So either is fine to me.
I think you mean make-mode is used by default. This was changed in Sphinx 1.6, I think? This affects the non-make-mode change. Users won't see it yet, but they will when I deprecate make mode in the future 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@shimizukawa do you have any opinion? I will merge this before 1.7 feature freeze. |
+0. I think that changing the old Makefile conflicts with the meaning of the |
Okay, let's go forward. |
To be clear, I'm hoping to do the opposite. I want to remove (well, deprecate and later remove) make mode by making |
I have to say no to you. We should keep make-mode for a while to keep compatibility. Of course, I also feel make-mode is not good. So I can understand and agree with you to remove it in future version. But is is not during 1.x. (I can't say it is okay in 2.0) So all we can do now is refactoring it simple or creating new CLI (called |
Oh, of course. I'm talking long term. We can't break users. I would expect a major version bump before we can remove it.
I'm working on this. Expect a pull request by tomorrow 😄 |
Subject: Add
epilog
attribute tosphinx.builder.Builder
Feature or Bugfix
Purpose
The
Makefile
,make.bat
, andsphinx-build -M
(make mode) tools all output very similar messages on successful build. Reduce this duplication (and the need for the latter) by moving this into the builders themselves.Detail
builders: Add
Builder.epilog
optionThis allows builders to emit a final epilog message containing information such as where resulting files can be found. This is only emitted if the build was successful.
This allows us to remove this content from the
make_mode
tool and the legacyMakefile
andmake.bat
templates. There's room for more dramatic simplification of the former, but this will come later.make_mode
: Remove unnecessarymake_*
functionsThese are handled by the default case.
Makefile
: Remove unnecessary targetsMost of these are not necessary now that we're not printing different messages for various builders.
make.bat
: Remove unnecessary targetsAs with the Makefile previously, these are not necessary now that we're not printing anything different for various builders.
Makefile
: MakeSOURCEDIR
configurableIt's unlikely that anyone will need to do this but at least give them the opportunity.
make.bat
: MakeSOURCEDIR
configurableIt's unlikely that anyone will need to do this but at least give them the opportunity.