Add url arg to the CLI #947

aar2dee2 · 2022-01-28T08:27:48Z

Issue #635

@jonatanklosko
We've added a :notebook clause to opts_to_config/2 to handle the :notebook switch. This returns opts with {:open, true} added so that open_from_options/2 is called. We're modifying the root_path to point to the notebook specified by the user. Please let me know if any edits to be made.

@ryg-git thank you for working on this with me :)

@ryg-git

@ryg-git - I added the notebook option in the usage/0 function (line 38) and in switches (line 142). The code for opening a notebook directly can be included in the `open_from_options/2` function, I think. We can have a third if-else clause for `opts[:notebook]`, append the filename to base url and then use Livebook.Utils.browser_open(). What do you think?

@ryg-git

@ryg-git I've added the :notebook clause for opts_to_config/2 (line 217). Since we want to open the notebook after changing the root path, and the `--open` flag may not be entered by user when using `--notebook`, I'm adding `{:open, true}` in the `opts` list. I've removed the if-else clause for `opts[:notebook]` added earlier in open_from_options/1, since that would be incorrect as the `:notebook` flag is not of boolean type.

add colon to make root_path an atom

CLAassistant · 2022-01-28T08:27:57Z

All committers have signed the CLA.

lib/livebook_cli/server.ex

jonatanklosko · 2022-01-28T12:54:45Z

Hey @aar2dee2 and @ryg-git, thanks for working on this! Just left a comment, let me know if anything is not clear :)

Add if-else clause to handle `:notebook` switch in open_from_options/2. Imports the notebook for now, rather than opening it.

aar2dee2 · 2022-01-28T14:04:25Z

@jonatanklosko
made some edits based on your comment. does this do as intended?

jonatanklosko · 2022-01-28T14:12:52Z

lib/livebook_cli/server.ex

@@ -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


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?

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.

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.

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.

Hopefully @josevalim has a strong preference 🙈

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?

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?

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?

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.

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.

lib/livebook_cli/server.ex

jonatanklosko · 2022-01-28T14:16:41Z

@aar2dee2 yup that's the right direction, just a few tiny comments :)

Co-authored-by: Jonatan Kłosko <[email protected]>

added as helper function to aid `:notebook` flag in Livebook CLI

lib/livebook_cli/server.ex

Co-authored-by: Jonatan Kłosko <[email protected]>

lib/livebook_web/helpers.ex

lib/livebook_cli/server.ex

lib/livebook_web/helpers.ex

Co-authored-by: José Valim <[email protected]>

lib/livebook_cli/server.ex

josevalim · 2022-01-29T08:59:06Z

lib/livebook_cli/server.ex

@@ -67,7 +62,11 @@ defmodule LivebookCLI.Server do
    case check_endpoint_availability(base_url) do
      :livebook_running ->
        IO.puts("Livebook already running on #{base_url}")
-        open_from_options(base_url, opts, extra_args)
+        if length(exta_args) > 1 do
+          print_error("Too many arguments entered. Please ensure only one argument is used to specify the file path and all other arguments are preceded by the relevant switch")


Instead of handling it here, you can handle it as a catch-all on open_from_options. This way there is no duplication either.

lib/livebook_cli/server.ex

josevalim · 2022-01-29T09:36:35Z

💚 💙 💜 💛 ❤️

aar2dee2 · 2022-01-29T09:40:32Z

enjoyed working on this. thanks a lot for guiding me :)

💚 💙 💜 💛 ❤️

ryg-git · 2022-01-29T17:46:14Z

@aar2dee2 thank you for doing all of the work, I am sorry I couldn't be of much help as I was busy in some personal work, thanks.

aar2dee2 · 2022-01-30T13:39:50Z

@aar2dee2 thank you for doing all of the work, I am sorry I couldn't be of much help as I was busy in some personal work, thanks.

@ryg-git no worries. hope to work with you again :)

aar2dee2 added 4 commits January 27, 2022 17:24

correct typo in root_path

16f0db3

add colon to make root_path an atom

correct File.cwd() to File.cwd!()

0ed2d4a

jonatanklosko reviewed Jan 28, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

Add :notebook option in open_from_options/2

f0ad72a

Add if-else clause to handle `:notebook` switch in open_from_options/2. Imports the notebook for now, rather than opening it.

jonatanklosko reviewed Jan 28, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

jonatanklosko reviewed Jan 28, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

jonatanklosko reviewed Jan 28, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

aar2dee2 and others added 3 commits January 28, 2022 19:47

Update lib/livebook_cli/server.ex

063d3c6

Co-authored-by: Jonatan Kłosko <[email protected]>

add function to open notebook on given path

609883b

added as helper function to aid `:notebook` flag in Livebook CLI

call notebook_import_url/1 from LivebookWeb.Helpers

7a52c74

jonatanklosko reviewed Jan 28, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

aar2dee2 and others added 2 commits January 28, 2022 20:28

Update lib/livebook_cli/server.ex

1122b49

Co-authored-by: Jonatan Kłosko <[email protected]>

modify option parser to return argv, import file if argv exists

0b40789

josevalim reviewed Jan 29, 2022

View reviewed changes

lib/livebook_web/helpers.ex Outdated Show resolved Hide resolved

josevalim reviewed Jan 29, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

josevalim reviewed Jan 29, 2022

View reviewed changes

lib/livebook_web/helpers.ex Outdated Show resolved Hide resolved

aar2dee2 and others added 3 commits January 29, 2022 14:14

fix indentation

e921650

Co-authored-by: José Valim <[email protected]>

add function doc

7024e43

Co-authored-by: José Valim <[email protected]>

modify open_from_options/3

105a9de

Co-authored-by: José Valim <[email protected]>

josevalim reviewed Jan 29, 2022

View reviewed changes

lib/livebook_cli/server.ex Show resolved Hide resolved

josevalim and others added 2 commits January 29, 2022 09:54

Update docs

f672986

return error if too many arguments given without relevant switch

83a1a07

josevalim reviewed Jan 29, 2022

View reviewed changes

aar2dee2 added 2 commits January 29, 2022 14:37

catch all clause in open_from_options/3 for too many arguments

85e0020

call notebook_import_url from LivebookWeb.Helpers

a436f60

josevalim reviewed Jan 29, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

josevalim reviewed Jan 29, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

josevalim reviewed Jan 29, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

josevalim and others added 4 commits January 29, 2022 10:19

mix format

a2853f5

Update server.ex

7756dd5

Merge branch 'main' into main

0382bd3

Update helpers.ex

e7cd6f7

josevalim reviewed Jan 29, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

Apply suggestions from code review

799ec3c

josevalim changed the title ~~add --notebook option in livebook CLI~~ Add url arg to the CLI Jan 29, 2022

josevalim reviewed Jan 29, 2022

View reviewed changes

lib/livebook_cli/server.ex Outdated Show resolved Hide resolved

josevalim added 2 commits January 29, 2022 10:28

Update lib/livebook_cli/server.ex

557ce3d

Update server.ex

b10c32c

josevalim merged commit 4202840 into livebook-dev:main Jan 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add url arg to the CLI #947

Add url arg to the CLI #947

aar2dee2 commented Jan 28, 2022

CLAassistant commented Jan 28, 2022 •

edited

Loading

jonatanklosko commented Jan 28, 2022

aar2dee2 commented Jan 28, 2022 •

edited

Loading

jonatanklosko Jan 28, 2022

wojtekmach Jan 28, 2022

jonatanklosko Jan 28, 2022

wojtekmach Jan 28, 2022

jonatanklosko Jan 28, 2022

jonatanklosko Jan 28, 2022 •

edited

Loading

josevalim Jan 28, 2022

jonatanklosko Jan 28, 2022

wojtekmach Jan 28, 2022 •

edited

Loading

jonatanklosko Jan 28, 2022

jonatanklosko commented Jan 28, 2022

josevalim Jan 29, 2022

aar2dee2 Jan 29, 2022

josevalim commented Jan 29, 2022

aar2dee2 commented Jan 29, 2022

ryg-git commented Jan 29, 2022

aar2dee2 commented Jan 30, 2022

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

Add url arg to the CLI #947

Add url arg to the CLI #947

Conversation

aar2dee2 commented Jan 28, 2022

CLAassistant commented Jan 28, 2022 • edited Loading

jonatanklosko commented Jan 28, 2022

aar2dee2 commented Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatanklosko Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtekmach Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatanklosko commented Jan 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Jan 29, 2022

aar2dee2 commented Jan 29, 2022

ryg-git commented Jan 29, 2022

aar2dee2 commented Jan 30, 2022

CLAassistant commented Jan 28, 2022 •

edited

Loading

aar2dee2 commented Jan 28, 2022 •

edited

Loading

jonatanklosko Jan 28, 2022 •

edited

Loading

wojtekmach Jan 28, 2022 •

edited

Loading