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

add auto port detection as in jupyter notebook #424

Merged
merged 1 commit into from Oct 14, 2019
Merged

add auto port detection as in jupyter notebook #424

merged 1 commit into from Oct 14, 2019

Conversation

katsar0v
Copy link
Contributor

Fixes #392 :)

@jtpio
Copy link
Member

jtpio commented Oct 13, 2019

Thanks @katsar0v for looking at this.

voila/app.py Outdated
break

if not success:
self.log.critical(_('ERROR: the notebook server could not be started because '
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to say "voila" here.

Copy link
Contributor Author

@katsar0v katsar0v Oct 13, 2019

Choose a reason for hiding this comment

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

I made a new commit and squashed the old ones :)

Add missing import in app.py

Update app.py - fix e128 indentation flake8 rule

Update app.py - flake8 e124 compatibility

rename notebook to voila in the critical message
Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks @katsar0v, tested locally and it works great.

As mentioned in #392 (comment), this is supposed to be replaced by #270 in the long run.
Until then it's still a useful thing to have.

Leaving it open for a while in case someone has an objection.

Comment on lines +526 to +528
break

if not success:
Copy link
Member

Choose a reason for hiding this comment

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

could be a single else. e.g.

for port in ...
   ...
else:
  ...

Copy link
Member

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

It is not tested in CI, but since indeed it will be replaced in the future, I'm ok with that. Happy to have this

@jtpio jtpio merged commit 3c6eccf into voila-dashboards:master Oct 14, 2019
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.

Autodetection of free port
3 participants