Skip to content
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

Merged
merged 9 commits into from May 19, 2017
Merged

Nicer error handling (Fix #2771) #2774

merged 9 commits into from May 19, 2017

Conversation

@ralsina
Copy link
Member

@ralsina ralsina commented May 18, 2017

Fix #2771

  • Log before raising exceptions
  • Eat the exception at the top level unless NIKOLA_DEBUG is set.
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
Copy link
Member

@Kwpolska Kwpolska May 18, 2017

The removal happens inside the if.

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

@Kwpolska Kwpolska May 18, 2017

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

Copy link
Member Author

@ralsina ralsina May 18, 2017

Good point

Copy link
Member

@Kwpolska Kwpolska May 18, 2017

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

try:
return super(DoitNikola, self).run(cmd_args)
except Exception:
pass
Copy link
Member

@Kwpolska Kwpolska May 18, 2017

return 1

# 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'
Copy link
Member

@Kwpolska Kwpolska May 18, 2017

I don’t think this applies to Nikola… We default to help via magic.

Copy link
Member Author

@ralsina ralsina May 18, 2017

Turns out it does.

@ralsina
Copy link
Member Author

@ralsina ralsina commented May 18, 2017

@@ -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:
Copy link
Member

@Kwpolska Kwpolska May 18, 2017

Is copy-pasting code really necessary for this? Would sys.exit(1) hurt here?

Copy link
Member Author

@ralsina ralsina May 18, 2017

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

Copy link
Member Author

@ralsina ralsina May 18, 2017

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

Copy link
Member

@Kwpolska Kwpolska May 18, 2017

Is that really a bare except:?

Copy link
Member Author

@ralsina ralsina May 18, 2017

Ok, fixed.

@ralsina
Copy link
Member Author

@ralsina ralsina commented May 18, 2017

@ralsina
Copy link
Member Author

@ralsina ralsina commented May 18, 2017

Copy link
Member

@Kwpolska Kwpolska left a comment

LGTM

@ralsina ralsina merged commit b657585 into master May 19, 2017
5 checks passed
@ralsina ralsina deleted the fix-2771 branch May 19, 2017
@@ -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)
Copy link
Contributor

@felixfontein felixfontein May 19, 2017

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.

Copy link
Member

@Kwpolska Kwpolska May 20, 2017

Added a conditional for debug in fc39108.

Copy link
Contributor

@felixfontein felixfontein May 20, 2017

Thanks!

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 20, 2017

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
Copy link
Member Author

@ralsina ralsina commented May 20, 2017

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
Copy link
Member

@Kwpolska Kwpolska commented May 20, 2017

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
Copy link
Contributor

@felixfontein felixfontein commented May 20, 2017

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.

Kwpolska added a commit that referenced this issue May 21, 2017
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
ralsina added a commit that referenced this issue May 21, 2017
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants