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

Respect the raise_config_file_errors trait and re-raise if applicable. #149

Merged
merged 3 commits into from
Jun 14, 2019

Conversation

ian-r-rose
Copy link
Member

I was taking a look at jupyterlab/jupyterlab#5716, and found that setting the base Application trait raise_config_file_errors wasn't behaving as I expected. This is because JupyterApp overloads the load_config_file function and doesn't check this option.

A quick audit of the Jupyter ecosystem shows that there are a couple of different behaviors around this.

  • jupyterhub inherits from Application rather than JupyterApp, so the trait works as expected.
  • IPython overrides load_config_file, and specifically checks for this option.
  • Notebook and JupyterLab inherit from JupyterApp, and they do not respect this option.

Another option would be to override the load_config_file option in lab/notebook, but it seemed cleaner to me to add this check here.

@ian-r-rose
Copy link
Member Author

Failure is due to setuptools no longer supporting python 3.3 (as of June 2018).

@ian-r-rose ian-r-rose closed this May 6, 2019
@ian-r-rose ian-r-rose reopened this May 6, 2019
@ian-r-rose
Copy link
Member Author

Ping @takluyver, any thoughts on this?

Copy link
Member

@takluyver takluyver left a comment

Choose a reason for hiding this comment

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

Thanks, this looks fine, just one minor suggestion for code clarity.

jupyter_core/application.py Outdated Show resolved Hide resolved
@takluyver takluyver added this to the 4.5 milestone Jun 13, 2019
@takluyver takluyver merged commit cf8cad9 into jupyter:master Jun 14, 2019
@takluyver
Copy link
Member

Thanks

@ian-r-rose
Copy link
Member Author

Thanks for the review + merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants