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.

Sign up for GitHub

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

Jump to bottom

PR: Add a new preferred-dir traitlet #549

Merged
merged 2 commits into from
Jul 8, 2021
Merged

PR: Add a new preferred-dir traitlet #549

merged 2 commits into from
Jul 8, 2021

Conversation

goanpeca
Copy link
Contributor

@goanpeca goanpeca commented Jul 7, 2021 •
edited
Loading

Fixes #534

  • Add a new trait to represent the "preferred" or "starting" directory (e.g., preferred_dir)
  • preferred_dir
    • will default to root_dir (aka notebook_dir)
    • is required to be an existing directory residing at or within root_dir
    • is a "suggestion" to client applications. Client applications can honor preferred_dir and start their file-browser at preferred_dir instead of root_dir.
    • If root_dir is updated and preferred dir is no longer a subdir or equal to root_dir, then it will be updated to use the root_dir and emit a warning.

@welcome
Copy link

welcome bot commented Jul 7, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@goanpeca goanpeca force-pushed the enh/add-preferred-dir-traitlet branch from 9a641a3 to b5f62ad Compare July 7, 2021 00:14
@goanpeca goanpeca force-pushed the enh/add-preferred-dir-traitlet branch from b0da0eb to c030c2d Compare July 7, 2021 00:19
@goanpeca
Copy link
Contributor Author

goanpeca commented Jul 7, 2021

@kevin-bates, I think I covered the approach you suggested. Let me know if it works. Thanks!

Zsailer
Zsailer approved these changes Jul 7, 2021
View reviewed changes
Copy link
Member

@Zsailer Zsailer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you, @goanpeca! This looks good to me (assuming the tests pass)!

@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2021 •
edited
Loading

Codecov Report

Merging #549 (04459a2) into master (6f970c5) will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #549      +/-   ##
==========================================
+ Coverage   76.34%   76.54%   +0.20%     
==========================================
  Files         110      110              
  Lines        9819     9918      +99     
  Branches     1067     1072       +5     
==========================================
+ Hits         7496     7592      +96     
- Misses       1943     1946       +3     
  Partials      380      380
Impacted Files Coverage Δ
jupyter_server/serverapp.py 65.52% <100.00%> (+0.56%) ⬆️
jupyter_server/tests/test_serverapp.py 99.41% <100.00%> (+0.32%) ⬆️
jupyter_server/extension/application.py 71.63% <0.00%> (-1.05%) ⬇️
jupyter_server/tests/test_utils.py 100.00% <0.00%> (ø)
jupyter_server/utils.py 64.77% <0.00%> (+1.62%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f970c5...04459a2. Read the comment docs.

kevin-bates
kevin-bates reviewed Jul 7, 2021
View reviewed changes
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

This looks good Gonzalo. Just had a couple of naming/typo nits. Thanks.

jupyter_server/serverapp.py Outdated
if not os.path.isdir(value):
raise TraitError(trans.gettext("No such directory: '%r'") % value)
return value

preferred_dir = Unicode(config=True,
help=trans.gettext("Prefered starting directory to use for notebooks and kernels.")
Copy link
Member

Choose a reason for hiding this comment

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

Typo:

Suggested change
help=trans.gettext("Prefered starting directory to use for notebooks and kernels.")
help=trans.gettext("Preferred starting directory to use for notebooks and kernels.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

jupyter_server/serverapp.py Outdated
@validate('root_dir')
def _root_dir_validate(self, proposal):
value = proposal['value']
def _normalize_root_or_preferred_dir(self, value):
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this name more generic since it doesn't require/act on root_dir or preferred_dir?

Suggested change
def _normalize_root_or_preferred_dir(self, value):
def _normalize_dir(self, value):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@goanpeca goanpeca requested a review from kevin-bates July 7, 2021 21:51
@goanpeca
Copy link
Contributor Author

goanpeca commented Jul 7, 2021

Made the fixes @kevin-bates, thanks :)

kevin-bates
kevin-bates approved these changes Jul 8, 2021
View reviewed changes
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Looks good Gonzalo - thank you.

@kevin-bates
Copy link
Member

The build failure is unrelated and being investigated - merging.

@kevin-bates kevin-bates merged commit f7290dc into jupyter-server:master Jul 8, 2021
@welcome
Copy link

welcome bot commented Jul 8, 2021

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@goanpeca goanpeca deleted the enh/add-preferred-dir-traitlet branch July 8, 2021 20:57
@krassowski krassowski mentioned this pull request Jul 13, 2021
Open
This was referenced Jul 21, 2021
Merged
Merged
@krassowski krassowski mentioned this pull request Dec 20, 2021
Closed
@vidartf vidartf mentioned this pull request Sep 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zsailer Zsailer Zsailer approved these changes

@kevin-bates kevin-bates kevin-bates approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Access files outside of notebook-dir
4 participants
@goanpeca @codecov-commenter @kevin-bates @Zsailer