Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

harmless but annoying bug in amber console #352

Closed
sam0x17 opened this issue Nov 2, 2017 · 17 comments
Closed

harmless but annoying bug in amber console #352

sam0x17 opened this issue Nov 2, 2017 · 17 comments

Comments

@sam0x17
Copy link
Contributor

sam0x17 commented Nov 2, 2017

  1. create a blank app and create some model, let's say it's called User, and give it an attribute called 'email' with a unique constraint on it
  2. run amber console
  3. run u = User.new(name: 'Sam', email: 'test@test.com')
  4. run u.save
  5. run u -- you will notice the u that gets printed has a unique constraint violation error on it, even though the save was actually successful. If you then run u = User.first, you will see the correctly saved user, without any constraint violation.
@eliasjpr
Copy link
Contributor

eliasjpr commented Nov 8, 2017

@sam0x17 Hello, sorry that you find it annoying. I would like for you to know that we also find it rather annoying.

The ICR shard is the root cause of this behavior. ICR shard basically writes the input statement to file on disk, and for each new statement it will append to the same file causing it to re run all previous statements.

We are putting some thoughts on how we can address this issue, and would like your input on ideas you might have around this.

Thank you for submitting the issue.

@faustinoaq
Copy link
Contributor

Hi, First Idea:

  • use built-in crystal play instead, crystal is compiled language, not interpreted. So need to recompile every time file changes. We can use playground and work-spaces with amber/granite/project libraries loaded by default or something similar.

@sam0x17
Copy link
Contributor Author

sam0x17 commented Nov 8, 2017

@eliasjpr yikes! that makes sense.

My guess is there is something that could be done in a way that is similar to how in certain scenarios C++ can dynamically load compiled libraries at runtime --- basically we need some way of compiling something and then "injecting" it into an already running scope/context....... 🤔

https://stackoverflow.com/questions/11016078/is-it-possible-to-create-a-function-dynamically-during-runtime-in-c

Like JIT basically

@sam0x17
Copy link
Contributor Author

sam0x17 commented Nov 8, 2017

another hackier possibility that might be a lot easier to implement: prefix lines you don't want ICR to re-run with some sort of marker, so like:

> u = User.find(1)
> ~ u.first_name = "Sam"
> ~ u.save
> u

Where ~ means that ICR should not re-run those lines when new lines are added.. i.e. ~ means "run this line only once"

I know with the way I work in the Ruby console I'd be able to pretty easily adapt my workflow to this, but it still is a bit confusing probably

@sam0x17
Copy link
Contributor Author

sam0x17 commented Nov 8, 2017

Now that I know this is what is going on, however, I will probably just do very long one-liners with lots of semicolons. Might be good to have some sort of warning printed when ICR loads explaining that every time you enter something all previous lines are re-executed

@elorest
Copy link
Member

elorest commented Nov 8, 2017

Just check context for every modifying action.
u = User.find(1) || User.create(email: "asdjfajsdf@sjasd.com", etc)

@oprypin
Copy link

oprypin commented Nov 8, 2017

I'm not sure why there is a need to expose a fake REPL that horribly breaks in almost all scenarios that would be used in a web framework's interactive console (e.g. networking, databases).

Why do we even have that lever?

Please just remove amber console.

@sam0x17
Copy link
Contributor Author

sam0x17 commented Nov 8, 2017

@oprypin point taken. I think the main motivation is the desire to have a way of quickly executing crystal code in a particular environment (e.g. play with things in your database, etc) without having to create an initializer or using something similar to a rake task just to run a simple one liner.

You make a very good point that anything stateful is not going to work and will be completely broken because ICR is basically a hack once you have multi-line code. I totally accept that. Unfortunately people coming from ruby/rails are really going to want some way of running one-liners.

Given that, a totally workable solution I think would be something like amber exec [insert crystal one-liner here], which would then load the default environment and execute the specified crystal code, printing the result to the terminal. Thoughts?

@faustinoaq
Copy link
Contributor

faustinoaq commented Nov 8, 2017

Hi, Why not use playground instead?. I mean, keeping amber console like an alias for crystal play

We can add an playground folder with an project_name.cr inside:

├── playground
│   └── myproyect.cr
├── public
│   ├── crossdomain.xml
│   ├── dist
│   └── robots.txt
├── README.md

myproject.cr has just one line require "../config/*"

Then, you go to http://localhost:8080/workbook/playground/myproject

screenshot_20171108_182039

And It's a crystal built-in feature.

@sam0x17
Copy link
Contributor Author

sam0x17 commented Nov 8, 2017

@faustinoaq glad to hear that including the environment is that simple. The reason I don't like crystal play as a solution is I can't do it from within a production docker image or on a live production server, as doing so would be exposing root control of the app to anyone smart enough to check port 8080. Also from a purely devops perspective, it would be super convenient to just be able to go amber exec [crystal code here].

Now that I know how environment loading works based on what you said, I'd be happy to develop this feature

@oprypin
Copy link

oprypin commented Nov 8, 2017

@faustinoaq I generally really like crystal play, but wouldn't it fail the use case described in this issue for the exact same reason? It can re-run your code without explicitly clicking "run". There is a UI setting to disable that, but I don't think there's a way for Amber to pre-configure that automatically.

@faustinoaq
Copy link
Contributor

amber exec [crystal code here]

@sam0x17 Yeah, It would be an alias to crystal eval something like:
crystal eval 'require "../config/*"; <your code here>' so:

$ amber exec <<-QUERY
> u = User.find(1)
> u.first_name = "Sam"
> u.save
> QUERY
Done!

@oprypin Yeah, the issue remains, but at least is clear we shouldn't use ICR nor amber console on production environments, so amber exec or amber eval aka crystal eval 'require "../config/*"; <your code here>' could be an option. 😄

@elorest
Copy link
Member

elorest commented Nov 10, 2017

I propose that we replace amber console with amber exec... which we maybe call amber console since e is already taken. :( WDYGT?

@sam0x17
Copy link
Contributor Author

sam0x17 commented Nov 10, 2017

@elorest I think calling it console might be a bit confusing since it isn't a REPL. I would call it amber run but that's already a thing which leaves execute/perform/exe/exec/... I actually liked your solution of doing -ex as the short version, since e is already taken. I'd be totally OK with renaming it from amber exec to amber execute if there is consensus for that, though it is longer to type... so imo we are stuck with exec but would love to hear what everyone else thinks

@faustinoaq
Copy link
Contributor

We could add amber build as well, like an alias for crystal build src/my_amber_project.cr

@sam0x17
Copy link
Contributor Author

sam0x17 commented Nov 10, 2017

@faustinoaq yes I think I suggested exactly that a while ago... maybe in chat? 🤔

I think it would be very useful. One of the biggest surprises I've had working with amber is that including the entire environment is really just as simple as a single require statement

elorest pushed a commit that referenced this issue Nov 11, 2017
* 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
@eliasjpr
Copy link
Contributor

eliasjpr commented Nov 14, 2017

Resolved with amber exec #383

elorest pushed a commit that referenced this issue Nov 17, 2017
* 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
elorest pushed a commit that referenced this issue Nov 17, 2017
* 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
elorest added a commit that referenced this issue Nov 17, 2017
@faustinoaq faustinoaq added this to Done in Framework 2018 Mar 17, 2018
@faustinoaq faustinoaq removed this from Done in Framework 2018 Jun 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

5 participants