-
Notifications
You must be signed in to change notification settings - Fork 460
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
Nicer error handling (Fix #2771) #2774
Conversation
nikola/nikola.py
Outdated
@@ -1086,8 +1086,7 @@ def init_plugins(self, commands_only=False, load_all=False): | |||
# Remove blacklisted plugins | |||
if p[-1].name in self.config['DISABLED_PLUGINS']: | |||
bad_candidates.add(p) | |||
utils.LOGGER.debug('Not loading disabled plugin {}', p[-1].name) | |||
# Remove compilers we don't use | |||
utils.LOGGER.debug('Not loading disabled plugin {}', p[-1].name) # Remove compilers we don't use |
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.
The removal happens inside the if
.
nikola/__main__.py
Outdated
LOGGER.error('Error loading tasks') | ||
if self.nikola.debug: | ||
raise | ||
return {}, {} |
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.
Won’t this make .doit.db empty or anything?
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.
Good point
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.
Perhaps just let the exception bubble upwards and handle it somewhere else.
nikola/__main__.py
Outdated
try: | ||
return super(DoitNikola, self).run(cmd_args) | ||
except Exception: | ||
pass |
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.
return 1
nikola/__main__.py
Outdated
# get specified sub-command or use default='run' | ||
if len(args) == 0 or args[0] not in sub_cmds: | ||
specified_run = False | ||
cmd_name = 'run' |
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 don’t think this applies to Nikola… We default to help
via magic.
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.
Turns out it does.
Well, it still does. I'll remove this.
…On Thu, May 18, 2017 at 12:10 PM Chris Warrick ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In nikola/__main__.py
<#2774 (comment)>:
> @@ -363,7 +371,41 @@ def run(self, cmd_args):
LOGGER.error("This command needs to run inside an "
"existing Nikola site.")
return 3
- return super(DoitNikola, self).run(cmd_args)
+ try:
+
+ # get "global vars" from cmd-line
+ args = self.process_args(cmd_args)
+
+ # get specified sub-command or use default='run'
+ if len(args) == 0 or args[0] not in sub_cmds:
+ specified_run = False
+ cmd_name = 'run'
I don’t think this applies to Nikola… We default to help via magic.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2774 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAGK4NhQ2mFRfxObFxQtTcQml6E4a_Iks5r7F90gaJpZM4NfVWp>
.
|
@@ -363,7 +371,33 @@ def run(self, cmd_args): | |||
LOGGER.error("This command needs to run inside an " | |||
"existing Nikola site.") | |||
return 3 | |||
return super(DoitNikola, self).run(cmd_args) | |||
try: |
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 copy-pasting code really necessary for this? Would sys.exit(1)
hurt here?
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.
Looking at it better, I would have to exit(1) in the next layer, since this run() prints the exceptions.
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.
Hell, even that won't work, it will catch exit() !!!!
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 that really a bare except:
?
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, fixed.
That is an alternative, yes. If you prefer that I can change.
…On Thu, May 18, 2017 at 12:14 PM Chris Warrick ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In nikola/__main__.py
<#2774 (comment)>:
> @@ -363,7 +371,33 @@ def run(self, cmd_args):
LOGGER.error("This command needs to run inside an "
"existing Nikola site.")
return 3
- return super(DoitNikola, self).run(cmd_args)
+ try:
Is copy-pasting code really necessary for this? Would sys.exit(1) hurt
here?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2774 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAGK4GWPxxwFxKicEwTOzrNFpNR7iH8ks5r7GBLgaJpZM4NfVWp>
.
|
Turns out no, it's an "except Exception:" so it would work. Will do it
tomorrow, probably.
…On Thu, May 18, 2017 at 1:39 PM Chris Warrick ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In nikola/__main__.py
<#2774 (comment)>:
> @@ -363,7 +371,33 @@ def run(self, cmd_args):
LOGGER.error("This command needs to run inside an "
"existing Nikola site.")
return 3
- return super(DoitNikola, self).run(cmd_args)
+ try:
Is that really a bare except:?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2774 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAGKyKiv5VlVnde0YMO9URqii6k9loRks5r7HRIgaJpZM4NfVWp>
.
|
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
nikola/__main__.py
Outdated
@@ -289,7 +285,7 @@ def load_tasks(self, cmd, opt_values, pos_args): | |||
signal('initialized').send(self.nikola) | |||
except Exception: | |||
LOGGER.error('Error loading tasks') | |||
raise | |||
sys.exit(3) |
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.
This is terrible: in case something happens, there is no way to see WHAT happened without modifying Nikola's source code! I've defined NIKOLA_DEBUG
, but I still just get Error loading tasks
.
I had to undo this change to get some useful output.
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.
Added a conditional for debug in fc39108.
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.
Thanks!
Hm, could we re-discuss this? Because in some cases, we will end up printing “Error loading tasks” without any explanation. How would people know what to fix? How would bug reports look? |
In that case, clearly we are raising exceptions that we are not logging, right? So, for that, we use NIKOLA_DEBUG=1 Also, I will check a little to see if we still have unlogging raises. |
It doesn’t have to be Nikola who’s raising those exceptions. My unhelpful errors were caused by Mako syntax errors. We should at least print the exception text, and tell the user how to get full debug info. |
Yes, a bit more information would be really helpful! It can always happen that we forget to write a message at some point in the future, and then some user gets a really unhelpful error message. Another thing which might be worth discussing: how about printing a short information text that you can get debug info with |
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Fix #2771