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

Add the tinker command #116

Merged
merged 2 commits into from
May 8, 2018
Merged

Add the tinker command #116

merged 2 commits into from
May 8, 2018

Conversation

abramisola
Copy link
Contributor

@abramisola abramisola commented May 8, 2018

Added this out of inspiration. I've based this off of develop so that we may have more discussion and push it out with the 2.0 release.

In essence, the craft tinker command is nothing more special than a python console with the container pre-loaded as a variable app. What is nice about it is that it allows us to more easily tinker around with the internals of the application either for testing or for seeding information before creating the store routes in our app. For instance you could do the following to retrieve all users.

>>> User = app.make('User')
>>> user = User.all()
>>> for user in users:
...     print(user.id)
...
1
2
3
4

For beginners, this will be a valuable tool for them to be able to play around with the container in an interactive way.

If there's anything else you want me to import by default into this console, that can easily be done.

@abramisola abramisola added enhancement New feature or request requires-discussion This is open for discussion. All ideas are welcome. labels May 8, 2018
@abramisola abramisola self-assigned this May 8, 2018
@coveralls
Copy link

coveralls commented May 8, 2018

Pull Request Test Coverage Report for Build 581

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 85.947%

Files with Coverage Reduction New Missed Lines %
masonite/testsuite/TestSuite.py 1 96.88%
Totals Coverage Status
Change from base Build 564: 0.02%
Covered Lines: 1321
Relevant Lines: 1537

💛 - Coveralls

mapeveri
mapeveri previously approved these changes May 8, 2018
Copy link
Contributor

@mapeveri mapeveri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, very useful to use in the console. @josephmancuso What do you think?

@josephmancuso
Copy link
Member

No way. @mapeveri suggested this a while ago but I couldn't figure out how to do this lol. I'm gonna review now

Copy link
Member

@josephmancuso josephmancuso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be added to the container. It currently won't be accessible when running craft. All commands are registered in the AppProvider register method

@abramisola
Copy link
Contributor Author

Whoops. Fixed. It's in the container now.

@josephmancuso
Copy link
Member

wow this is awesome.

@josephmancuso
Copy link
Member

are there other things we should load in?

@abramisola
Copy link
Contributor Author

I'm not sure. I was thinking that it might be helpful to automatically load in the models, but I figured many people would be putting those into the container anyway, so no real need.

If we had more facades, then maybe those as well, but since there's only one, I'm not sure if it is particularly useful.

@josephmancuso
Copy link
Member

very true. Yeah if people need things loaded in they should just make a service provider to do it.

@josephmancuso josephmancuso merged commit 89cf7cc into develop May 8, 2018
@abramisola abramisola deleted the create-tinker-command branch May 10, 2018 01:27
@josephmancuso josephmancuso mentioned this pull request Jun 12, 2018
27 tasks
@clsource clsource mentioned this pull request Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request requires-discussion This is open for discussion. All ideas are welcome.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants