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

Improve error message when NotebookRunner initialized with a String #705

Merged
merged 1 commit into from
Apr 16, 2022

Conversation

Zhenye-Na
Copy link
Contributor

Description

This CR includes changes that make NotebookRunner initialized with a String explicitly throw an exception if the string looks like a pathlib.Path

Tests

$ pytest tests/sources/test_notebooksource.py -k 'test_error_if_source_str_like_path'
============================================================ test session starts ============================================================
platform linux -- Python 3.7.11, pytest-7.1.1, pluggy-1.0.0
rootdir: /home/vagrant/ploomber, configfile: pyproject.toml
plugins: cov-3.0.0, anyio-3.5.0
collected 87 items / 86 deselected / 1 selected

tests/sources/test_notebooksource.py .                                                                                                [100%]

================================================ 1 passed, 86 deselected, 1 warning in 0.32s ================================================
$ pytest tests/tasks/test_notebook.py  -k 'test_initialize_with_str_like_path'
============================================================ test session starts ============================================================
platform linux -- Python 3.7.11, pytest-7.1.1, pluggy-1.0.0
rootdir: /home/vagrant/ploomber, configfile: pyproject.toml
plugins: cov-3.0.0, anyio-3.5.0
collected 69 items / 68 deselected / 1 selected

tests/tasks/test_notebook.py .                                                                                                        [100%]

================================================ 1 passed, 68 deselected, 1 warning in 0.12s ================================================

Related Issue

#694

@edublancas
Copy link
Contributor

thanks for working on this!

at a quick glance, the code looks good.

can you check flake8, I'm seeing this in the CI

./tests/tasks/test_notebook.py:971:1: E302 expected 2 blank lines, found 1

https://github.com/ploomber/ploomber/blob/master/CONTRIBUTING.md#submitting-code

if Path(self._primitive).exists():
raise ValueError(
'{} looks like a path to a file. '
'Perhaps you meant passing a pathlib.Path object'.format(
Copy link
Contributor

@edublancas edublancas Apr 11, 2022

Choose a reason for hiding this comment

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

please show a code example as part of the error message:

perhaps you meant passing a pathlib.Path object like this:

from pathlib import Path
NotebookRunner(Path('value-that-the-user-passed'))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please show a code example as part of the error message:

sorry, I dont get the question.

Copy link
Contributor

Choose a reason for hiding this comment

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

to be more user-friendly, modify the error message so we tell the user how to fix their code (otherwise they need to figure out that they need to add an import and then enclose their string using Path. it'll make it easier for them to know how to fix it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see your point. make sense to me.

Let me update the error message in the next revision

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

@Zhenye-Na
Copy link
Contributor Author

thanks for working on this!

at a quick glance, the code looks good.

can you check flake8, I'm seeing this in the CI

./tests/tasks/test_notebook.py:971:1: E302 expected 2 blank lines, found 1

https://github.com/ploomber/ploomber/blob/master/CONTRIBUTING.md#submitting-code

Yes, it seems like I did not run flake8 in the project level. fixing it

@edublancas edublancas merged commit 5cdaffc into ploomber:master Apr 16, 2022
@edublancas
Copy link
Contributor

thanks for your contribution!

neelasha23 pushed a commit to neelasha23/ploomber that referenced this pull request May 22, 2022
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.

None yet

2 participants