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

Add url arg to the CLI #947

Merged
merged 24 commits into from
Jan 29, 2022
Merged

Add url arg to the CLI #947

Changes from 5 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
58b09f2
add notebook option in usage function and switches
aar2dee2 Jan 27, 2022
62db327
Add :notebook clause for opts_to_config/2
aar2dee2 Jan 28, 2022
16f0db3
correct typo in `root_path`
aar2dee2 Jan 28, 2022
0ed2d4a
correct `File.cwd()` to `File.cwd!()`
aar2dee2 Jan 28, 2022
f0ad72a
Add `:notebook` option in open_from_options/2
aar2dee2 Jan 28, 2022
063d3c6
Update lib/livebook_cli/server.ex
aar2dee2 Jan 28, 2022
609883b
add function to open notebook on given path
aar2dee2 Jan 28, 2022
7a52c74
call `notebook_import_url/1` from LivebookWeb.Helpers
aar2dee2 Jan 28, 2022
1122b49
Update lib/livebook_cli/server.ex
aar2dee2 Jan 28, 2022
0b40789
modify option parser to return argv, import file if argv exists
aar2dee2 Jan 29, 2022
e921650
fix indentation
aar2dee2 Jan 29, 2022
7024e43
add function doc
aar2dee2 Jan 29, 2022
105a9de
modify open_from_options/3
aar2dee2 Jan 29, 2022
f672986
Update docs
josevalim Jan 29, 2022
83a1a07
return error if too many arguments given without relevant switch
aar2dee2 Jan 29, 2022
85e0020
catch all clause in open_from_options/3 for too many arguments
aar2dee2 Jan 29, 2022
a436f60
call notebook_import_url from LivebookWeb.Helpers
aar2dee2 Jan 29, 2022
a2853f5
mix format
josevalim Jan 29, 2022
7756dd5
Update server.ex
josevalim Jan 29, 2022
0382bd3
Merge branch 'main' into main
josevalim Jan 29, 2022
e7cd6f7
Update helpers.ex
josevalim Jan 29, 2022
799ec3c
Apply suggestions from code review
josevalim Jan 29, 2022
557ce3d
Update lib/livebook_cli/server.ex
josevalim Jan 29, 2022
b10c32c
Update server.ex
josevalim Jan 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lib/livebook_cli/server.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ defmodule LivebookCLI.Server do
--name Set a name for the app distributed node
--no-token Disable token authentication, enabled by default
If LIVEBOOK_PASSWORD is set, it takes precedence over token auth
--notebook Open browser window pointing to notebook specified
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--notebook Open browser window pointing to notebook specified
--open-notebook Open browser window pointing to the given notebook

A bit longer, but consistent with the other ones. If someone uses this frequently, they are likely to setup an alias anyway, @josevalim any preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

my unsolicited opinion is +1, I was gonna propose the same. Another idea is --open-file. It's a separate discussion but would it make sense to have $ livebook server --open-url https://github.com/... too? If so perhaps --open-file makes a bit more sense.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, we could make --open-notebook accept both file path and URL, not sure if that's better or worse. Note that once we do #927 then the semantics would be different, from URL we just import, from file we actually open, but that's a sidenote.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think we could check whether given argument starts with http(s):// (maybe we special case livebook:// too, dunno, probably not as it's supposed to be internal) and if so it's an url, otherwise it's a local file. I kinda like the more explicit --open-file & --open-url but a single option that handles both might be something more users would expect.

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully @josevalim has a strong preference 🙈

Copy link
Member

@jonatanklosko jonatanklosko Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So I'm thinking we can add --open-file and --import-url in this PR, but both of them import, then in a follow up PR we add an endpoint for opening local files and use it for --open-file and #927. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, to be honest, I am not sure. This sounds verbose. What if instead we make this an argument to livebook server? Such as livebook server Foo.livemd? And if a URL is given, we import?

then this PR supports only URLs and we add files later?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, we also have this option. The good thing is that we avoid the ambiguous open/import wording altogether, so it's definitely less confusing. The only question is if we would want to use argv for anything else in the future, but I cannot think of an obvious case. @wojtekmach?

Copy link
Contributor

@wojtekmach wojtekmach Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Cant think of any other use case for argv. Yet another option, to not paint ourselves into a wall, is a more generic —-url URL, a file path is an url after all too. But maybe it could be mistaken for the url of the server and not of the thing we want to access. So yeah argv sounds good to me.

Copy link
Member

Choose a reason for hiding this comment

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

Fantastic, let's do argv then!

@aar2dee2 sorry for revisiting just now 🙈 To sum up, the idea is to have livebook server [options] [.livemd url], so we need to get argv from option parser and if there is anything we will import the URL.

Then in a separate PR we will extend it to [.livemd file or url], and when file is given we "Open" it instead of importing.

--open Open browser window pointing to the application
--open-new Open browser window pointing to a new notebook
-p, --port The port to start the web application on, defaults to 8080
Expand Down Expand Up @@ -126,6 +127,10 @@ defmodule LivebookCLI.Server do
|> append_path("/explore/notebooks/new")
|> Livebook.Utils.browser_open()
end

if Keyword.has_key?(opts, :notebook) do
jonatanklosko marked this conversation as resolved.
Show resolved Hide resolved
LivebookApp.import_livebook("file://" <> File.cwd!() <> Keyword.fetch!(opts, :notebook))
jonatanklosko marked this conversation as resolved.
Show resolved Hide resolved
end
end
josevalim marked this conversation as resolved.
Show resolved Hide resolved

@switches [
Expand All @@ -134,6 +139,7 @@ defmodule LivebookCLI.Server do
default_runtime: :string,
ip: :string,
name: :string,
notebook: :string,
open: :boolean,
open_new: :boolean,
port: :integer,
Expand Down Expand Up @@ -210,7 +216,7 @@ defmodule LivebookCLI.Server do
autosave_path = Livebook.Config.autosave_path!("--autosave-path", path)
opts_to_config(opts, [{:livebook, :autosave_path, autosave_path} | config])
end

aar2dee2 marked this conversation as resolved.
Show resolved Hide resolved
defp opts_to_config([_opt | opts], config), do: opts_to_config(opts, config)
josevalim marked this conversation as resolved.
Show resolved Hide resolved

defp append_path(url, path) do
Expand Down