crystal-community / icr

Interactive console for Crystal programming language
MIT License
505 stars 40 forks source link

Usage warning #66

Closed veelenga closed 7 years ago

veelenga commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 https://github.com/crystal-community/icr/issues/65#issuecomment-336852916

jwoertink commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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" notifications@github.com 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 https://github.com/crystal-community/icr/pull/66#issuecomment-337958833, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG7aLEtPJ6lX69SdVfFQedry4J-EAV1ks5st3VOgaJpZM4P-NIL .

veelenga commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

veelenga commented 7 years ago

Thank you guys for participation and support ;)