cli: add "amber exec" command for executing one-liners (#352) #371

sam0x17 · 2017-11-09T05:10:41Z

Description of the Change

Adds a simple CLI command for executing one-liner pieces of crystal code within the application scope and printing the result.

Syntax:

$ amber exec 'puts User.first'
#<User:0xc04f80>
$ amber x # opens file and runs it on save/exit.
$ amber x -b 1 # runs the previous saved command.
$ amber x file.cr # copies file contents to new tmp file opens in editor and runs on exit.

update:

now supports amber encrypt-style syntax using your preferred text editor
now supports specifying a .cr file instead of code

related issue #352

Alternate Designs

ICR: bad because it's a fake REPL and can cause unintended side effects since all lines are re-run each time a new line is added
reworking amber play: bad because it requires a web browser and suffers from similar re-running issues

Benefits

extremely simple
enables effective devops by allowing code to be executed against the preferred amber environment
e.g. you could do AMBER_ENV=production amber exec 'code here'
does not suffer from code-rerunning issues, since we are only running what you can cram onto a single

Possible Drawbacks

implementation would be better if it automatically detected whether or not you are currently in an amber project directory, instead of assuming you are in the root of a project. I'm open to suggestions on how we should handle this
would be cool if we could cache some of the compilation somehow so successive calls to amber exec only need to re-compile changed files, but this might be overkill

Testing

I have manually tested this on my system. I'd be happy to write unit tests -- not familiar with specs in amber but I can dive in and figure it out if required. If someone could provide some high level advice on writing specs for this I'd greatly appreciate it (i.e. testing this command requires a dummy project skeleton to be in place).

elorest

You also probably need to load the models.

sam0x17 · 2017-11-09T05:14:22Z

@elorest interestingly it seems I don't, at least in the app I'm testing e.g.:

$ amber exec "u = User.first; puts u.first_name if u"
Sam

elorest · 2017-11-09T05:15:04Z

I was planning on building a concept similar to this that opened up an editor and let you type whatever code you wanted. On exit it would run and print the output. Similar in a way to edit in pry but with the REPL.

sam0x17 · 2017-11-09T05:15:48Z

@elorest I really like that idea, so maybe we should just wait for you to do that

elorest · 2017-11-09T05:17:37Z

This command could probably do both of those. The specs in Amber are just using crystal spec. Fairly easy to wrap your head around. Look at some of the other CLI specs. And copy them. You really just need to verify that calling it returns the output you expect.

sam0x17 · 2017-11-09T05:20:05Z

@elorest kind of the way git commit works?

elorest · 2017-11-09T05:29:29Z

Yeah pretty much.

…

On Nov 8, 2017 10:20 PM, "Sam Johnson" ***@***.***> wrote: @elorest <https://github.com/elorest> kind of the way git commit works? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#371 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHmpOSHO9i8PJe_vQ4bwkB5LbX7evdnks5s0ouGgaJpZM4QXcbu> .

elorest · 2017-11-09T05:29:41Z

I'd default it to vim but let them pass any editor command in as an option.

…

On Nov 8, 2017 10:28 PM, ***@***.*** wrote: @elorest <https://github.com/elorest> kind of the way git commit works? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#371 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHmpOSHO9i8PJe_vQ4bwkB5LbX7evdnks5s0ouGgaJpZM4QXcbu> .

sam0x17 · 2017-11-09T05:57:36Z

@elorest I'd agree with that but I think it also might be nice maybe as a separate PR to make it so that you can specify an AMBER_EDITOR environment variable so amber encrypt and this could use whatever editor without having to specify each time. nano guy here heh

veelenga · 2017-11-09T06:31:26Z

@elorest @sam0x17 i was trying to build a similar concept. You should give it a look https://github.com/veelenga/vicr. I often use it to answer questions on SO :)

Brew installation is broken, but you can just run it locally with make build && ./out/vicr

sam0x17 · 2017-11-09T06:38:59Z

@elorest ok so maybe the zero-arg would open the editor, and if you provide an arg then it works as above -- I'll see what I can do and base it partially off what (I'm guessing you?) did for for amber encrypt

@veelenga I'll check it out for sure!

elorest · 2017-11-09T19:17:25Z

Yeah you could also look at an ENV var but if not ENV or flag is provided I would default to vim.

ENV["EDITOR"] || options.editor

elorest · 2017-11-09T19:18:27Z

@veelenga What I'm thinking of is really simple, maybe like 5 additional lines of code. I'll look at that code you linked though.

eliasjpr · 2017-11-09T19:35:16Z

Just throwing this to see what you guys think

Workflow:
1 - amber exec -e nvim (This creates a hidden cr file something like .console.cr)
2 - Edit (Add code remove code etc)
3 - saves and build printing results

I basically did a proof of concept, kinda within an amber project dir
1 - touch .console.cr & nvim ./.console.cr
2 - Added puts "Hello World!" of course
3 - crystal build ./.console.cr
4 ./.console => "Hello World!"

If you happen to type amber exec again it will open the previous session. I would have to .gitignore the .console.cr file.

With this exec who needs a console! Nice work @sam0x17

@veelenga I really like what you did!

sam0x17 · 2017-11-10T01:51:31Z

@eliasjpr thanks -- they way I dealt with that particular issue if you look at my force-push from last night is I delete the file right after it is compiled, so no need to add it to .gitignore since it only exists on the filesystem for a few milliseconds.

You will notice I also added support for doing a one-liner inline without opening the editor, and I added support for specifying a path to an existing .cr file. I'm doing some specs for this tonight so after that should be ready to review.

Note that this .cr file detection is performed by checking if the argument to amber exec is a string ending in '.cr', and if it is, it checks if it is a file that exists, otherwise the argument is treated as crystal code. I've been using this for a day now and it seems to work quite well.

sam0x17 · 2017-11-10T01:55:54Z

side note: I would be curious to know what trick ICR is using to pretty print models from granite-orm. e.g. in ICR when I print a model, I get all the attributes, but with this I just get <ModelName:memory_address>

sam0x17 · 2017-11-10T02:16:00Z

@eliasjpr do you think it would be useful to include a --persist option that will create the behavior you describe above where the file stays after executing so you can re-edit it and potentially run again?

elorest · 2017-11-10T04:26:39Z

@sam0x17

side note: I would be curious to know what trick ICR is using to pretty print models from granite-orm. e.g. in ICR when I print a model, I get all the attributes, but with this I just get ModelName:memory_address

Use model.inspect to get all the information.

@eliasjpr do you think it would be useful to include a --persist option that will create the behavior you describe above where the file stays after executing so you can re-edit it and potentially run again?

My suggestion would be to add temp files to ./tmp/console_[timestamp].cr. This is already added to the .gitignore. That way you could have a back flag and run the files
from before. amber exec --back 1 or amber exec --back 2 to run last one or one before that.

faustinoaq · 2017-11-10T04:38:00Z

my suggestion would be to add temp files to ./tmp/console_[timestamp].cr

@elorest I think we can use https://crystal-lang.org/api/0.23.1/Tempfile.html here

sam0x17 · 2017-11-10T04:42:03Z

@faustinoaq @elorest ok I'll change it to use Tempfile -- btw just pushed changes that add the --persist option I spoke of

elorest · 2017-11-10T04:51:57Z

@sam0x17 I don't think that a persist option is the best route. Just store all files with a timestamp in ./tmp That way anyone can be accessed with a flag like with pry in ruby.

sam0x17 · 2017-11-10T05:03:10Z

@elorest that makes sense -- feel free to remove the --persist option in your commit when you add the timestamp stuff

sam0x17 · 2017-11-10T05:15:30Z

thoughts about enclosing the code for one-liners in a puts (code).inspect e.g. crystal eval 'require "../config/*"; puts (#{args.code}).inspect' so that it behaves more like a one-liner in ICR?

I just went to use amber exec a few mins ago and forgot it won't print anything unless I explicitly puts

…k#352)

marksiemers

Looks good overall - just have the spec file wrapped in the check for the build flag, and add that context wrapper.

marksiemers · 2017-11-11T04:55:39Z

spec/amber/cli/commands/exec_spec.cr

+  describe "amber exec" do
+    cleanup
+    scaffold_app(TESTING_APP)
+    `shards`


If this test requires a build, it should be wrapped checking for the run_build_tests flag.

Like this: https://github.com/amberframework/amber/pull/354/files#diff-5fff94200507b682866348a325e3bd88R1

{% if flag?(:run_build_tests) %}

Sorry, I forgot about one of the implications of this, you have to change a line in the Dockerfile to make these tests run on Travis. Right now, the test suite is a false green because the tests for this PR are skipped.

https://github.com/sam0x17/amber/blob/amber-exec/Dockerfile#L9
Needs to be CMD ["crystal", "spec", "-D", "run_build_tests"]

It has been a pending change from here: https://github.com/amberframework/amber/pull/354/files#diff-654a8a2c7db020a0d9cf8c58982c3864R9

That's the big Spec PR that has been held up for various reasons.

You're breaking my heart man.

marksiemers · 2017-11-11T04:59:35Z

spec/amber/cli/commands/exec_spec.cr

+      # This one weird trick, to make the specs pass in linux leaves scientists puzzled.
+      Dir.glob("tmp/*")
+      logs = Dir.glob("./tmp/*_console_result.log")
+      File.read(logs.last?.to_s).should eq expected_result


We get more helpful failures, rather than errors with a nil check:

logs.last?.should_not be nil (File.read(logs.last?.to_s).should eq expected_result) if logs.last?

This change is optional.

marksiemers · 2017-11-11T05:03:23Z

spec/amber/cli/commands/exec_spec.cr

+      Dir.glob("tmp/*")
+      logs = Dir.glob("./tmp/*_console_result.log")
+      File.read(logs.last?.to_s).should eq "1000\n"
+    end


Wrap the two above with context "with edit flag '-e'" do ... end

marksiemers · 2017-11-11T06:10:06Z

spec/amber/cli/commands/exec_spec.cr

+      MainCommand.run(["exec", "-e", "tail", "-b", "1"])
+      logs = `ls tmp/*_console_result.log`.strip.split(/\s/)
+      File.read(logs.last?.to_s).should eq "1000\n"
+    end


Wrap lines 28-38 in a context "in editor mode" do ... end

…#371)" This reverts commit 8dce363.

…#371)" (#382) This reverts commit 8dce363.

* cli: add "amber exec" command for executing one-liners (#352) * this now behaves as close to a console as we can make it * finishing touches for amber exec * do not run editor unless in editor mode * always return a result * minor wording change in error message * amber exec specs * cleaned up code a bit and copied existing file to new temp file for edits * specs pass yay. MainCommand.run always returns nil which makes testing hard * new push to see if travis passes. Pass locally so I'm confused. * debugging to figure out why travis doesn't pass * added sleeps to see if that works * one last thing * what the what? * i hope this will clear it up * weird hack that i think fixes specs on linux * commented weird hack * native ls had better work * formatted and fixed tests probably * removed some vestigual code. * removed macro flag for now * test47 of travis * namespaces specs * sorted ls results

…#371)" (#382) This reverts commit 8dce363.

* cli: add "amber exec" command for executing one-liners (#352) * this now behaves as close to a console as we can make it * finishing touches for amber exec * do not run editor unless in editor mode * always return a result * minor wording change in error message * amber exec specs * cleaned up code a bit and copied existing file to new temp file for edits * specs pass yay. MainCommand.run always returns nil which makes testing hard * new push to see if travis passes. Pass locally so I'm confused. * debugging to figure out why travis doesn't pass * added sleeps to see if that works * one last thing * what the what? * i hope this will clear it up * weird hack that i think fixes specs on linux * commented weird hack * native ls had better work * formatted and fixed tests probably * removed some vestigual code. * removed macro flag for now * test47 of travis * namespaces specs * sorted ls results Former-commit-id: 3a55d2a

…#371)" (#382) This reverts commit 137c95b [formerly 8dce363]. Former-commit-id: af11eeb

elorest reviewed Nov 9, 2017

View reviewed changes

sam0x17 force-pushed the amber-exec branch 3 times, most recently from b7e9e02 to 18a8bda Compare November 9, 2017 09:03

sam0x17 force-pushed the amber-exec branch from 78826b6 to 14359a6 Compare November 10, 2017 01:51

sam0x17 force-pushed the amber-exec branch from 8bdbe6c to c37e626 Compare November 10, 2017 04:39

cli: add "amber exec" command for executing one-liners (amberframewor…

561ad25

…k#352)

sam0x17 force-pushed the amber-exec branch from c37e626 to 561ad25 Compare November 10, 2017 05:26

elorest added 6 commits November 10, 2017 20:29

added sleeps to see if that works

85b5fd6

one last thing

0928fe6

what the what?

4d20886

i hope this will clear it up

4672a3f

weird hack that i think fixes specs on linux

ad812cc

commented weird hack

2737797

marksiemers suggested changes Nov 11, 2017

View reviewed changes

elorest added 4 commits November 10, 2017 22:29

native ls had better work

3ea29d5

formatted and fixed tests probably

491c71e

removed some vestigual code.

e66e782

removed macro flag for now

5d3c3df

marksiemers reviewed Nov 11, 2017

View reviewed changes

elorest added 3 commits November 10, 2017 23:18

test47 of travis

1a5e0ba

namespaces specs

9f720f3

sorted ls results

2b1bb15

marksiemers approved these changes Nov 11, 2017

View reviewed changes

elorest merged commit 8dce363 into amberframework:master Nov 11, 2017

elorest added a commit that referenced this pull request Nov 11, 2017

Revert "cli: add "amber exec" command for executing one-liners (#352) (…

e14fd87

…#371)" This reverts commit 8dce363.

elorest mentioned this pull request Nov 11, 2017

Revert "cli: add "amber exec" command for executing one-liners (#352)" #382

Merged

elorest added a commit that referenced this pull request Nov 11, 2017

Revert "cli: add "amber exec" command for executing one-liners (#352) (…

557b79a

…#371)" (#382) This reverts commit 8dce363.

This was referenced Nov 12, 2017

Document merged PRs from the last month. #384

Closed

Pull Requests which need to be documented before release. #385

Closed

elorest added a commit that referenced this pull request Nov 17, 2017

Revert "cli: add "amber exec" command for executing one-liners (#352) (…

af11eeb

…#371)" (#382) This reverts commit 8dce363.

elorest added a commit that referenced this pull request Nov 17, 2017

Revert "cli: add "amber exec" command for executing one-liners (#352) (…

a730d15

…#371)" (#382) This reverts commit 137c95b [formerly 8dce363]. Former-commit-id: af11eeb

veelenga mentioned this pull request Jan 10, 2018

Add elorest/cry veelenga/awesome-crystal#248

Merged

faustinoaq added this to Done in Framework 2018 May 5, 2018

faustinoaq removed this from Done in Framework 2018 Jun 12, 2018

faustinoaq added this to Done in Amber Framework 2017 Jun 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: add "amber exec" command for executing one-liners (#352) #371

cli: add "amber exec" command for executing one-liners (#352) #371

sam0x17 commented Nov 9, 2017 •

edited by elorest

elorest left a comment

sam0x17 commented Nov 9, 2017 •

edited

elorest commented Nov 9, 2017

sam0x17 commented Nov 9, 2017 •

edited

elorest commented Nov 9, 2017

sam0x17 commented Nov 9, 2017

elorest commented Nov 9, 2017 via email

elorest commented Nov 9, 2017 via email

sam0x17 commented Nov 9, 2017

veelenga commented Nov 9, 2017 •

edited

sam0x17 commented Nov 9, 2017

elorest commented Nov 9, 2017 •

edited

elorest commented Nov 9, 2017

eliasjpr commented Nov 9, 2017

sam0x17 commented Nov 10, 2017 •

edited

sam0x17 commented Nov 10, 2017 •

edited

sam0x17 commented Nov 10, 2017

elorest commented Nov 10, 2017 •

edited

faustinoaq commented Nov 10, 2017

sam0x17 commented Nov 10, 2017

elorest commented Nov 10, 2017

sam0x17 commented Nov 10, 2017

sam0x17 commented Nov 10, 2017 •

edited

marksiemers left a comment

marksiemers Nov 11, 2017

marksiemers Nov 11, 2017

elorest Nov 11, 2017

marksiemers Nov 11, 2017

marksiemers Nov 11, 2017

elorest Nov 11, 2017

marksiemers Nov 11, 2017

marksiemers Nov 11, 2017

marksiemers Nov 11, 2017

marksiemers Nov 11, 2017

cli: add "amber exec" command for executing one-liners (#352) #371

cli: add "amber exec" command for executing one-liners (#352) #371

Conversation

sam0x17 commented Nov 9, 2017 • edited by elorest

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Testing

elorest left a comment

Choose a reason for hiding this comment

sam0x17 commented Nov 9, 2017 • edited

elorest commented Nov 9, 2017

sam0x17 commented Nov 9, 2017 • edited

elorest commented Nov 9, 2017

sam0x17 commented Nov 9, 2017

elorest commented Nov 9, 2017 via email

elorest commented Nov 9, 2017 via email

sam0x17 commented Nov 9, 2017

veelenga commented Nov 9, 2017 • edited

sam0x17 commented Nov 9, 2017

elorest commented Nov 9, 2017 • edited

elorest commented Nov 9, 2017

eliasjpr commented Nov 9, 2017

sam0x17 commented Nov 10, 2017 • edited

sam0x17 commented Nov 10, 2017 • edited

sam0x17 commented Nov 10, 2017

elorest commented Nov 10, 2017 • edited

faustinoaq commented Nov 10, 2017

sam0x17 commented Nov 10, 2017

elorest commented Nov 10, 2017

sam0x17 commented Nov 10, 2017

sam0x17 commented Nov 10, 2017 • edited

marksiemers 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

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

sam0x17 commented Nov 9, 2017 •

edited by elorest

sam0x17 commented Nov 9, 2017 •

edited

sam0x17 commented Nov 9, 2017 •

edited

veelenga commented Nov 9, 2017 •

edited

elorest commented Nov 9, 2017 •

edited

sam0x17 commented Nov 10, 2017 •

edited

sam0x17 commented Nov 10, 2017 •

edited

elorest commented Nov 10, 2017 •

edited

sam0x17 commented Nov 10, 2017 •

edited