feature: optionally start a browser when executing madness about madness

[DannyBen]

Cool.

I don't mind continuing to explore this path - either by you continuing to experiment with it, or me.

In any case, here are my thoughts:

1. I believe the entire feature can be done with just a few lines of code. The larger it gets, the less attractive it is to add.
2. The flag to add to the command line should simply be --open.
3. There should be a similarly named setting in the config file.
4. The fork should only happen when --open is requested, otherwise - we preserve the exact same behavior as it is now.
5. As for error-proofing the command - I think it can be a simple rescue that displays a "Failed to open browser" message to STDOUT.

Let me know if you want me to take over, otherwise I am assuming you continue to tinker with it yourself.

[DannyBen]

Can you just start a draft PR, so we have an easier place to review all changes and discuss?

[DannyBen]

Closed via #88

[DannyBen]

Well, you mention two issues:

Default bind address

Madness wants to provide the user with the least friction possible.

I have seen (ans answered) dozens of issues on StackOverflow (not related to Madness) about people not understanding why their server is not accessible, with the answer being to change bind address to 0.0.0.0.

I am using Vagrant and Docker a lot, so 127.0.0.1 does not work in either of them. Also, Madness is intended for running documentation on your workstation - why are you allowing incoming port 80 traffic anyways?

You can specify the bind address in the command line, or, probably more suitable to your needs - in the config file.

I am leaning towards keeping this default, but still open to be persuaded otherwise.

Launching the browser

I am not strongly against such a feature, but I suspect it will add unnecessary complexity that is harder to test. Since it will:

1. Have to run the server as a fork, wait for it to load, and then open the URL.
2. Have to work on multiple systems, checking if xdg-open is installed, or try with open on mac.
3. Have to fail gracefully if it doesn't find it.

In general, I am not particularly fond of adding complexity when it can be solved with something as simple as:

#!/usr/bin/env bash
nohup madness > /dev/null &
sleep 3
open http://localhost:3000

If you believe you have a simple implementation that can work, I am open to taking a look at it and perhaps reconsider my position.

[DannyBen]

As a reference, here is a StackOverflow answer with a couple of solutions: One using a gem, and another homemade.

The Launchy gem seems abandoned though.

[fwolfst]

The 'os' gem https://github.com/rdp/os is the way to go, i believe. I might draft a PR later.

[DannyBen]

The 'os' gem https://github.com/rdp/os is the way to go

Hmm. Not sure about it at all.
If you already know what commands are used for what OS, why not simply check if the command exists? A-la duck-typing - if it has xdg-open I don't care what OS it is.... (and on the flip side, if I am on linux, does not mean I have xdg-open).

And on that note - madness already uses my Colsole gem as a runtime dependency, which means it already has a command_exist? command method.

[fwolfst]

If you already know what commands are used for what OS, why not simply check if the command exists?

True, but then you have to maintain that list and logic, besides that it adds complexity.

If I draft a PR, could I add bin/madness-now instead of a config flag (only difference: madness-now would try to open a browser via os)?

[DannyBen]

True, but then you have to maintain that list and logic, besides that it adds complexity.

Why - the os gem has open logic? I thought it only helps you determine the OS.
Oh - found it def self.open_file_command. Yes. This can work, but might still fail if the command does not exist.

If I draft a PR, could I add bin/madness-now instead

In the drafting stage, sure.

[fwolfst]

Why - the os gem has open logic? I thought it only helps you determine the OS.

https://stackoverflow.com/questions/152699/open-the-default-browser-in-ruby/58855763#58855763

It has

OS.open_file_command
=> "start" # or open on mac, or xdg-open on linux (all designed to open a file)

[fwolfst]

I did some prototyping in my branch fwolfst/madness , but I am convinced that its a good idea to walk that path (also because of the usage of docopt, which makes us repeat stuff in this particular case). I click the PR button if you want, but would leave stuff as-is otherwise. Thanks for your openness and willingness to think about that feature.

[DannyBen]

Well, I looked.

1. The location of the added code

The bin/* is not the place for it, since madness can be executed with some parameters that will not result in running a server.

The better place for it, is in the method that launches the server:

madness/lib/madness/command_line.rb

Lines 49 to 58 in bf135e3

	def launch_server
	unless File.directory? config.path
	STDERR.puts "Invalid path (#{config.path})"
	return
	end
	show_status
	Server.prepare
	Server.run!
	end

So - for prototyping or not - the above approach saves you the need to have another binary, and another docopt.

2. The additional os gem

... will need to be added to the gemspec

Do you want:

1. to continue working on it yourself.
2. to abandon it.
3. me to take a stab at it

?

[fwolfst]

The better place for it, is in the method that launches the server:

madness/lib/madness/command_line.rb

Lines 49 to 58 in bf135e3

	def launch_server
	unless File.directory? config.path
	STDERR.puts "Invalid path (#{config.path})"
	return
	end
	show_status
	Server.prepare
	Server.run!
	end

Thanks, I know. I ported my original patch:

    def launch_server
      unless File.directory? config.path
        STDERR.puts "Invalid path (#{config.path})" 
        return
      end

      show_status
      fork do
        Server.prepare
        Server.run!
      end
      `xdg-open http://localhost:3000`
      Process.wait
    end

over because I had the idea that a separate executable would overcome my wish to change the "default" behaviour (which you objected, also because of the docker image etc - and I can see your points).

2. The additional os gem

... will need to be added to the gemspec

Yeah, it already is ...

Do you want:

1. to continue working on it yourself.
2. to abandon it.
3. me to take a stab at it

If you are not overly fond of that feature, I would say lets abandon it.

If you think that all that is missing is e.g. a --and-open-browser flag and maybe a graceful exit/msg if OS does not provide a open_file_command or the command fails I can continue to work on it a bit.

[fwolfst]

3. There should be a similarly named setting in the config file.

Done, as setting 'open' for now (but untested). And I think the setting in the config file should be named differently, maybe as verbose as auto_open_browser.

5. As for error-proofing the command - I think it can be a simple `rescue` that displays a "Failed to open browser" message to STDOUT.

No big proofing done.

Let me know if you want me to take over, otherwise I am assuming you continue to tinker with it yourself.

I walked some steps, happy if you take over (tell me if there is really minor things and I should PR). Please also read the full commit messages (just a line or two).
I was about to make args an instance variable and then figured that you probably like passing it around. If not, I think this would be the time to change it now.

[fwolfst]

I will review the changes soon, functionally it looks fine. I honor all the time and careful consideration you put in it, although you wanted to keep the patch small and have probably more pressing issues.

However, looking at the current API and thinking about "how would I like the CLI API to look", I now consider following options superior (its always great when these ideas come up when the implementation is already done ... ):

# 1) tell dont ask: hey madness, open the docs! with subcommand
madness open doc/

# 2) be in line with --and-quit, also more verbose
madness --and-open-browser

[DannyBen]

# 1) tell dont ask: hey madness, open the docs! with subcommand
madness open doc/

# 2) be in line with --and-quit, also more verbose
madness --and-open-browser

No. I don't see it, sorry.

This is the general usage pattern:

Usage:
  madness [PATH] [options]
  madness create config
  madness create theme FOLDER

With the idea that:

1. The main purpose of madness is to run a server in a folder. Everything else is secondary. The folder is always the first argument, and the first argument is almost always the folder.
2. If you need to run pre-launch commands (like --index) - they are expressed as flags
3. Since you might want to do some pre-launch commands, without launching - I added --and-quit. Notice that the and is there, only since it is always intended to be used with another flag. So in English, it reads "madness index and quit".
4. The only place where I allowed myself to override the "the first argument is a folder", is where there are other administrative tasks (like create config), that are never associated with a server launch.
5. The flag --open is a common one when CLIs open browser.
6. In command line flags and options, I tend to stick to the shortest string possible, but not shorter. It just needs to conveys the meaning clearly enough enough, while still remaining quick and hopefully intuitive to type.

[fwolfst]

We disagree and its fine with me.

[DannyBen]

We disagree and its fine with me.

In the future, if --open might mean something else, or be confusing due to the addition of another feature, then there might be a reason to consider changing. For now, lets keep it as it.

[fwolfst]

Excellent, was fun working with you. Thanks.