Usage warning #66

veelenga · 2017-10-18T19:04:23Z

This change is about to warn user regarding usage issues in Readme and while running icr.

Short demo:

$ cat ~/.icr
cat: ~/.icr: No such file or directory

$ icr
WARNING: ICR is not a real REPL and may have side effects.
Please read the documentation carefully and be sure you understand how it works before using it.
Disable this warning with --disable-usage-warning.
icr(0.23.1) > ^D

$ icr
WARNING: ICR is not a real REPL and may have side effects.
Please read the documentation carefully and be sure you understand how it works before using it.
Disable this warning with --disable-usage-warning.
icr(0.23.1) > ^D

$ icr --disable-usage-warning
icr(0.23.1) > ^D

$ icr
icr(0.23.1) > ^D

$ cat ~/.icr
---
print_usage_warning: false

should close #65

/cc @jwoertink @greyblake @RX14 @oprypin @martinos

RX14 · 2017-10-18T20:21:18Z

First of all, lets follow the XDG spec on where to place the icr config file. Second of all, it's not immediately obvious to the user that --disable-usage-warning is sticky. You should make ICR exit with no repl and a "Usage warning disabled" message once --disable-usage-warning is passed.

oprypin · 2017-10-18T20:34:03Z

If you're gonna add an annoying message AND go through the trouble of adding this setting file, better make it count and ensure that people will actually read it instead of sending them to some vague location.
No references. Include everything from #65 (comment)

greyblake

Cool! Thanks for taking care of it!

jwoertink · 2017-10-19T15:50:01Z

src/icr/settings.cr

+require "yaml"
+
+class Icr::Settings
+  PATH = File.expand_path "~/.icr"


Can we follow the run commands convention with this filename and make it .icrrc? That would follow other files like .irbrc, .gemrc, .bashrc, .vimrc, etc... We could easily take advantage of having this file for other settings we might want to add.

rc stays for run commands, but this one is actually configuration file..

Right. I was thinking that if this is a flag that gets run when you run icr, and the purpose of the file is to include the flag, it would make sense for it to be a run command. But if we're not going to use it like that, then I guess this is fine.

Please follow the XDG spec here and use the ${XDG_CONFIG_HOME:-~/.config}/icr/ folder.

I've actually never heard of XDG before. What other utilities can I look at that follow this spec so I can see what they are doing?

You'll probably find the spec if you search XDG_CONFIG_HOME. The idea is to read this environment variable (it's almost never set on a typical system) and fall back to $HOME/.config.

jwoertink · 2017-10-19T16:14:05Z

I'm not sure how I feel about plastering all over "Not a real REPL"... VS saying something like "Not a full featured REPL".

Really, we should just work out a plan to make it a full repl 😛

RX14

I think the XDG spec is a must for any modern application creating config files.

RX14 · 2017-10-19T23:12:22Z

Honestly if you use the XDG spec, you can simplify this to remove the config file and make it a single 0-byte flag file as you have a whole directory to play with. touch $XDG_CONFIG_HOME/icr/usage-warning-accepted etc.

oprypin

I still think this message in its current form is a waste of time. Basically same effect as "RTFM".
It really should be informative to be justified.

oprypin · 2017-10-19T23:26:28Z

How about this:

ICR is not a real REPL. It works by building up source code, compiling and re-running all of it after each inputted line. That has side effects, for example:

Current time and random numbers change retroactively.
Files and network sockets are reopened every time.

veelenga · 2017-10-20T06:52:08Z

Hey guys. Thanks for review and valuable comments.

@RX14 I know about XDG spec, which was proposed in 2010 and hasn't became a standard (has it?). Moreover, it confuses people on OS X (~/Library or ~/.config?). Windows is a different story. But I like the idea with 0-byte file, which will simplify the things. Also will follow your "second of all" and change the way --disable-usage-warning works :) Thank you!

@oprypin I like your suggestion regarding warning message. Good point, actually 👍

Will prepare changes during weekend, sorry for delay.

RX14 · 2017-10-20T07:27:27Z

@veelenga I don't know exactly what you mean by standard but its become very widely used on Linux. About half the applications I use follow the spec, and almost all of the modern ones do. I don't know about how osx handles dot files, but I thought it was the same?

veelenga · 2017-10-20T08:29:24Z

I don't know about how osx handles dot files, but I thought it was the same?

Right, same way. But as far as I know XDG suggests to use ~/Library/<App Name> on OS X (which is not actually a "dot" directory). And lot of apps write to ~/.<app-name> and some (modern ones :)) to ~/.config/<app-name>.

So this becomes a mess. Probably this is a lack of configuration on my system, but I expect it to work properly from scratch 😕

RX14 · 2017-10-20T08:41:35Z

@veelenga The advice i've seen is for command-line applications to stick to ~/.config and the other linux directories on OSX. Haskell's stdlib does this at least.

Perhaps we should create XDG helpers in the stdlib which handle this platform-specificness for you.

veelenga · 2017-10-20T09:10:02Z

@RX14 Does Haskell have it in stdlib? I think you are referring to xdg-basedir package. It works for me just like you said:

$ cabal repl                                                                                                                                                                      
Preprocessing library xdg-basedir-0.2.2...
GHCi, version 8.2.1: http://www.haskell.org/ghc/  :? for help
[1 of 1] Compiling System.Environment.XDG.BaseDir ( System/Environment/XDG/BaseDir.hs, interpreted )
Ok, 1 module loaded.

*System.Environment.XDG.BaseDir> System.Environment.XDG.BaseDir.getUserConfigDir("icr")
"/Users/_me_/.config/icr"

*System.Environment.XDG.BaseDir> System.Environment.XDG.BaseDir.getUserCacheDir("icr")
"/Users/_me_/.cache/icr"

*System.Environment.XDG.BaseDir> System.Environment.XDG.BaseDir.getAllDataDirs("icr")
["/Users/_me_/.local/share/icr","/usr/local/share/icr","/usr/share/icr"]

Would love to see similar stuff for Crystal 👍

RX14 · 2017-10-20T10:05:42Z

Actually this package: doc. But yes, lets just use the simple File.join(ENV["XDG_CONFIG_HOME"]? || "~/.config", "icr") on all platforms for now.

greyblake · 2017-10-20T12:38:48Z

I thought about this at the very beggining. For normal REPL Crystal needs to support incremental compilation. Some compiling languages like Haskell or Julia have REPL, so we may take a look how they do it..

…

On Oct 19, 2017 18:14, "Jeremy Woertink" ***@***.***> wrote: I'm not sure how I feel about plastering all over "Not a real REPL"... VS saying something like "Not a full featured REPL". Really, we should just work out a plan to make it a full repl 😛 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#66 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG7aLEtPJ6lX69SdVfFQedry4J-EAV1ks5st3VOgaJpZM4P-NIL> .

veelenga · 2017-10-20T18:09:00Z

So now:

$ rm ~/.config/icr/usage_warning_accepted
remove /Users/_me_/.config/icr/usage_warning_accepted? y

$ ./bin/icr
WARNING: ICR is not a full featured REPL.
It works by building up source file, compiling and re-running all of it on each input.
That has side effects:

  * Current time and random numbers change retroactively
  * Files and network/database connections are reopened on every run
  * Running a sleep or benchmark will delay an execution of next inputs
  * Unexpected behavior of fibers, channels, shell commands and maybe others

Be careful while running your commands.

You can disable this warning with --disable-usage-warning flag.
icr(0.23.1) > ^D

$ ./bin/icr
WARNING: ICR is not a full featured REPL.
It works by building up source file, compiling and re-running all of it on each input.
That has side effects:

  * Current time and random numbers change retroactively
  * Files and network/database connections are reopened on every run
  * Running a sleep or benchmark will delay an execution of next inputs
  * Unexpected behavior of fibers, channels, shell commands and maybe others

Be careful while running your commands.

You can disable this warning with --disable-usage-warning flag.
icr(0.23.1) > ^D

$ ./bin/icr --disable-usage-warning
Usage warning disabled. Run ICR again to continue.

$ ./bin/icr
icr(0.23.1) > ^D

$ ls ~/.config/icr/
usage_warning_accepted

jwoertink · 2017-10-20T18:11:04Z

I like that message better. Saying "Not a full featured REPL". I noticed I have the .config directory too with some stuff in it already, so it's not far fetched throwing some crystal config stuff in there 😅

I think it looks good.

veelenga · 2017-10-20T19:59:38Z

@RX14 @oprypin @greyblake just need your OK or other feedback :)

oprypin

You asked for it 😐
mkdir_p is the only absolutely necessary change.
OK otherwise.

oprypin · 2017-10-20T20:02:04Z

src/icr/cli.cr

+def print_usage_warning
+  puts <<-WARN
+  WARNING: ICR is not a full featured REPL.
+  It works by building up source file, compiling and re-running all of it on each input.


building up a source file?

oprypin · 2017-10-20T20:02:30Z

src/icr/cli.cr

+
+    * Current time and random numbers change retroactively
+    * Files and network/database connections are reopened on every run
+    * Running a sleep or benchmark will delay an execution of next inputs


delay an execution?

oprypin · 2017-10-20T20:04:00Z

src/icr/cli.cr

+
+  unless usage_warning_accepted
+    parser.on("--disable-usage-warning", "Disable usage warning") do
+      Dir.mkdir CONFIG_HOME unless Dir.exists? CONFIG_HOME


mkdir_p and no need to check for existence

oprypin · 2017-10-20T20:08:11Z

src/icr/cli.cr

@@ -32,7 +54,20 @@ OptionParser.parse! do |parser|
  parser.on("-r FILE", "--require=FILE", "auto require FILE") do |filename|
    libs.push(%{require "#{filename}"})
  end
+
+  unless usage_warning_accepted
+    parser.on("--disable-usage-warning", "Disable usage warning") do


Maybe don't make the application accept a different set of flags depending on some condition? Swap these two lines. Or even remove unless usage_warning_accepted (see other comment)

oprypin · 2017-10-20T20:10:34Z

src/icr/cli.cr

+      usage_warning_accepted = true
+
+      puts "Usage warning disabled. Run ICR again to continue."
+      exit 0


Why print this message and why quit? Just let it run as normal. It's more predictable. And, well, it's important for scriptable CLI tools to always have a way to not depend on global state. This is not a scriptable tool but still...

No. We shouldn't make it run as normal because it doesn't make it clear that the warning has been permenantly disabled. Install parallel and follow their "citation" procedure to see how it should be done.

veelenga · 2017-10-21T15:17:19Z

Thank you guys for participation and support ;)

veelenga requested review from greyblake and jwoertink October 18, 2017 19:05

Usage warning

5427ce3

veelenga force-pushed the feat/usage-warning branch from 5f3fab4 to 5427ce3 Compare October 18, 2017 19:07

greyblake approved these changes Oct 19, 2017

View reviewed changes

jwoertink requested changes Oct 19, 2017

View reviewed changes

jwoertink approved these changes Oct 19, 2017

View reviewed changes

RX14 suggested changes Oct 19, 2017

View reviewed changes

oprypin suggested changes Oct 19, 2017

View reviewed changes

veelenga added 2 commits October 20, 2017 20:39

Improve usage warning

733650d

Follow XDG spec

2d4a285

oprypin suggested changes Oct 20, 2017

View reviewed changes

Fix typos and avoid conditional cli arguments

78524a3

veelenga merged commit 960fe90 into master Oct 21, 2017

veelenga deleted the feat/usage-warning branch October 21, 2017 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Usage warning #66

Usage warning #66

veelenga commented Oct 18, 2017 •

edited

Loading

RX14 commented Oct 18, 2017

oprypin commented Oct 18, 2017

greyblake left a comment

jwoertink Oct 19, 2017

greyblake Oct 19, 2017

jwoertink Oct 19, 2017

RX14 Oct 19, 2017 •

edited

Loading

jwoertink Oct 19, 2017

oprypin Oct 20, 2017

jwoertink commented Oct 19, 2017

RX14 left a comment

RX14 commented Oct 19, 2017

oprypin left a comment •

edited

Loading

oprypin commented Oct 19, 2017

veelenga commented Oct 20, 2017 •

edited

Loading

RX14 commented Oct 20, 2017

veelenga commented Oct 20, 2017 •

edited

Loading

RX14 commented Oct 20, 2017 •

edited

Loading

veelenga commented Oct 20, 2017 •

edited

Loading

RX14 commented Oct 20, 2017 •

edited

Loading

greyblake commented Oct 20, 2017 via email

veelenga commented Oct 20, 2017

jwoertink commented Oct 20, 2017

veelenga commented Oct 20, 2017 •

edited

Loading

oprypin left a comment

oprypin Oct 20, 2017

oprypin Oct 20, 2017

oprypin Oct 20, 2017

oprypin Oct 20, 2017

oprypin Oct 20, 2017

RX14 Oct 20, 2017

veelenga commented Oct 21, 2017

Usage warning #66

Usage warning #66

Conversation

veelenga commented Oct 18, 2017 • edited Loading

RX14 commented Oct 18, 2017

oprypin commented Oct 18, 2017

greyblake left a 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

RX14 Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwoertink commented Oct 19, 2017

RX14 left a comment

Choose a reason for hiding this comment

RX14 commented Oct 19, 2017

oprypin left a comment • edited Loading

Choose a reason for hiding this comment

oprypin commented Oct 19, 2017

veelenga commented Oct 20, 2017 • edited Loading

RX14 commented Oct 20, 2017

veelenga commented Oct 20, 2017 • edited Loading

RX14 commented Oct 20, 2017 • edited Loading

veelenga commented Oct 20, 2017 • edited Loading

RX14 commented Oct 20, 2017 • edited Loading

greyblake commented Oct 20, 2017 via email

veelenga commented Oct 20, 2017

jwoertink commented Oct 20, 2017

veelenga commented Oct 20, 2017 • edited Loading

oprypin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veelenga commented Oct 21, 2017

veelenga commented Oct 18, 2017 •

edited

Loading

RX14 Oct 19, 2017 •

edited

Loading

oprypin left a comment •

edited

Loading

veelenga commented Oct 20, 2017 •

edited

Loading

veelenga commented Oct 20, 2017 •

edited

Loading

RX14 commented Oct 20, 2017 •

edited

Loading

veelenga commented Oct 20, 2017 •

edited

Loading

RX14 commented Oct 20, 2017 •

edited

Loading

veelenga commented Oct 20, 2017 •

edited

Loading