Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Improve error handling for missing files #2551

Merged
merged 2 commits into from Oct 17, 2017
Merged

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Oct 17, 2017

os.path.exists doesn't allow us to distinguish between permissions errors and
the path actually not existing, which repeatedly confuses people. It also means
that we try to overwrite existing key files, which is super-confusing. (cf
issues #2455, #2379). Use os.stat instead.

Also, don't recomemnd the the use of --generate-config, which screws everything
up if you're using debian (cf #2455).

`os.path.exists` doesn't allow us to distinguish between permissions errors and
the path actually not existing, which repeatedly confuses people. It also means
that we try to overwrite existing key files, which is super-confusing. (cf
issues #2455, #2379). Use os.stat instead.

Also, don't recomemnd the the use of --generate-config, which screws everything
up if you're using debian (cf #2455).
@erikjohnston
Copy link
Member

I'm a bit scared that we're losing some helpful hints for people tying to use python directly, or do we have error messages elsewhere if a config file isn't specified? Generally we can rewrite the help text in the debian build where appropriate (e.g. we already remove references to pip install)

def check_file(cls, file_path, config_name):
if file_path is None:
raise ConfigError(
"Missing config for %s."
" You must specify a path for the config file. You can "
Copy link
Member Author

Choose a reason for hiding this comment

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

this text was completely bogus, afaict. This function is called to check that files referenced from the config file exist, rather than the config file itself.

@richvdh
Copy link
Member Author

richvdh commented Oct 17, 2017

I'm a bit scared that we're losing some helpful hints for people tying to use python directly, or do we have error messages elsewhere if a config file isn't specified?

if no config file is specified, you hit

config_parser.error(
, which one could argue has a similar problem with the debian package, but at least you're less likely to hit it under debian than some of the others.

Of all the things caught by the messages I'm removing, I'm unconvinced that --generate-config will solve any of them rather than making everything worse.

@erikjohnston
Copy link
Member

Ok, then that works for me

@richvdh richvdh merged commit 7216c76 into develop Oct 17, 2017
@richvdh richvdh deleted the rav/keyfile_loading branch October 17, 2017 13:46
@richvdh richvdh mentioned this pull request Oct 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants