Nicer error handling (Fix #2771) #2774

ralsina · 2017-05-18T14:39:22Z

Log before raising exceptions
Eat the exception at the top level unless NIKOLA_DEBUG is set.

Kwpolska · 2017-05-18T14:42:46Z

nikola/nikola.py

@@ -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


The removal happens inside the if.

Kwpolska · 2017-05-18T14:43:36Z

nikola/__main__.py

+            LOGGER.error('Error loading tasks')
+            if self.nikola.debug:
+                raise
+            return {}, {}


Won’t this make .doit.db empty or anything?

Perhaps just let the exception bubble upwards and handle it somewhere else.

Kwpolska · 2017-05-18T14:46:18Z

nikola/__main__.py

+        try:
+            return super(DoitNikola, self).run(cmd_args)
+        except Exception:
+            pass


…ix-2771

Kwpolska · 2017-05-18T15:10:42Z

nikola/__main__.py

+            # 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.

Turns out it does.

ralsina · 2017-05-18T15:12:03Z

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> .

Kwpolska · 2017-05-18T15:14:19Z

nikola/__main__.py

@@ -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?

Looking at it better, I would have to exit(1) in the next layer, since this run() prints the exceptions.

Hell, even that won't work, it will catch exit() !!!!

Is that really a bare except:?

ralsina · 2017-05-18T15:38:26Z

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> .

ralsina · 2017-05-18T18:32:22Z

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> .

Kwpolska

LGTM

felixfontein · 2017-05-19T21:02:40Z

nikola/__main__.py

@@ -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)


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.

Added a conditional for debug in fc39108.

Kwpolska · 2017-05-20T18:37:02Z

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?

ralsina · 2017-05-20T19:23:59Z

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.

Kwpolska · 2017-05-20T19:25:43Z

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.

felixfontein · 2017-05-20T20:10:54Z

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 NIKOLA_DEBUG=1 in case an exception is caught? Then some more interested persons (power-users, hackers, ...) don't have to search the documentation (or the source code) first before being able to get more information.

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

…2782) * Show last exception text and NIKOLA_DEBUG explanation (#2774 #2771) Signed-off-by: Chris Warrick <kwpolska@gmail.com> * more friendly → friendlier Signed-off-by: Chris Warrick <kwpolska@gmail.com>

Roberto Alsina and others added 2 commits May 18, 2017 11:38

Nicer error handling (Fix #2771)

1f144c3

Merge branch 'master' into fix-2771

c5904c4

Kwpolska requested changes May 18, 2017

View reviewed changes

Kwpolska reviewed May 18, 2017

View reviewed changes

nikola/__main__.py Outdated

try:

return super(DoitNikola, self).run(cmd_args)

except Exception:

pass

Copy link

Member

Kwpolska May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return 1

Roberto Alsina added 4 commits May 18, 2017 11:46

Hmmm

efbe9fd

Merge branch 'fix-2772' of https://github.com/getnikola/nikola into f…

81dccdb

…ix-2771

return 1

4a690bb

Don't let doit do noisy exception printing

05ca9ab

Kwpolska reviewed May 18, 2017

View reviewed changes

We don't run

43cbf69

Kwpolska reviewed May 18, 2017

View reviewed changes

Ok, so that code does do something ;-)

02bfc27

Just exit(3) if can't load tasks

b657585

Kwpolska approved these changes May 19, 2017

View reviewed changes

ralsina merged commit b657585 into master May 19, 2017

ralsina deleted the fix-2771 branch May 19, 2017 14:55

felixfontein reviewed May 19, 2017

View reviewed changes

Kwpolska added a commit that referenced this pull request May 21, 2017

Show last exception text and NIKOLA_DEBUG explanation (#2774 #2771)

cc27a4a

Signed-off-by: Chris Warrick <kwpolska@gmail.com>

Kwpolska mentioned this pull request May 21, 2017

Show last exception text and NIKOLA_DEBUG explanation (#2774 #2771) #2782

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nicer error handling (Fix #2771) #2774

Nicer error handling (Fix #2771) #2774

ralsina commented May 18, 2017

Kwpolska May 18, 2017

Kwpolska May 18, 2017

ralsina May 18, 2017

Kwpolska May 18, 2017

Kwpolska May 18, 2017

Kwpolska May 18, 2017

ralsina May 18, 2017

ralsina commented May 18, 2017 via email

Kwpolska May 18, 2017

ralsina May 18, 2017

ralsina May 18, 2017

Kwpolska May 18, 2017

ralsina May 18, 2017

ralsina commented May 18, 2017 via email

ralsina commented May 18, 2017 via email

Kwpolska left a comment

felixfontein May 19, 2017

Kwpolska May 20, 2017

felixfontein May 20, 2017

Kwpolska commented May 20, 2017

ralsina commented May 20, 2017

Kwpolska commented May 20, 2017

felixfontein commented May 20, 2017

Nicer error handling (Fix #2771) #2774

Nicer error handling (Fix #2771) #2774

Conversation

ralsina commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralsina commented May 18, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralsina commented May 18, 2017 via email

ralsina commented May 18, 2017 via email

Kwpolska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kwpolska commented May 20, 2017

ralsina commented May 20, 2017

Kwpolska commented May 20, 2017

felixfontein commented May 20, 2017