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

app.run should be able to handle the output from controller commands #257

Merged
merged 2 commits into from Jul 9, 2014

Conversation

Projects
None yet
2 participants
@rocktavious
Contributor

rocktavious commented Jun 5, 2014

dispatch should return anything the controller commands return as well as the main app should try and render that output, if you override the render method you can control how and when that output is displayed, this way you can keep a lot of your controller commands as data in and data out and can easily be unit tested individually

Kyle Rockman
app.run should be able to handle the output from controller commands
dispatch should return anything the controller commands return as well as the main app should try and render that output, if you override the render method you can control how and when that output is displayed, this way you can keep a lot of your controller commands as data in and data out and can easily be unit tested
@rocktavious

This comment has been minimized.

Show comment
Hide comment
@rocktavious

rocktavious Jun 5, 2014

Contributor

Looking at the Travis CI build this ticket doesn't seem to have failed because of my changes but because of current failures in the tests. Please consider this for integration.

Contributor

rocktavious commented Jun 5, 2014

Looking at the Travis CI build this ticket doesn't seem to have failed because of my changes but because of current failures in the tests. Please consider this for integration.

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Jun 5, 2014

Member

Thanks @rocktavious I'm checking it out now.

Member

derks commented Jun 5, 2014

Thanks @rocktavious I'm checking it out now.

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Jun 5, 2014

Member

OK, so it looks pretty simple of a change, however it interferes with the existing structure in that it is not strictly required that an application controller return a dict() nor render that dict(). That is a use case issue... some people don't use app.render() ... some commands don't render any output ... others would are much easier to handle with simple print statements. We currently don't enforce output rendering.

I believe that this change actually did break one of the tests in Travis because that test is using app.get_last_rendered() which accesses the last result after calling "app.render()".

This is how we currently handle testing output.... we run a command, then check the output of that command in "app.get_last_rendered()". That might be helpful to you.

I don't see a problem with returning the func() from CementBaseController._dispatch() as that doesn't break existing functionality. You could then jus tmodify your app to call app.render() on it to simplify your work flow.

Member

derks commented Jun 5, 2014

OK, so it looks pretty simple of a change, however it interferes with the existing structure in that it is not strictly required that an application controller return a dict() nor render that dict(). That is a use case issue... some people don't use app.render() ... some commands don't render any output ... others would are much easier to handle with simple print statements. We currently don't enforce output rendering.

I believe that this change actually did break one of the tests in Travis because that test is using app.get_last_rendered() which accesses the last result after calling "app.render()".

This is how we currently handle testing output.... we run a command, then check the output of that command in "app.get_last_rendered()". That might be helpful to you.

I don't see a problem with returning the func() from CementBaseController._dispatch() as that doesn't break existing functionality. You could then jus tmodify your app to call app.render() on it to simplify your work flow.

@rocktavious

This comment has been minimized.

Show comment
Hide comment
@rocktavious

rocktavious Jun 5, 2014

Contributor

So basically just remove the changes from foundation.py and make that a special override in my local app class? and we'll keep the changes to controller.py?

Contributor

rocktavious commented Jun 5, 2014

So basically just remove the changes from foundation.py and make that a special override in my local app class? and we'll keep the changes to controller.py?

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Jun 5, 2014

Member

Would you mind updating your fork, so that I can merge it from you. Like you said, just drop the render() in foundation.py. I Would prefer to keep your commit history and contribution rather than making the changes myself.

Member

derks commented Jun 5, 2014

Would you mind updating your fork, so that I can merge it from you. Like you said, just drop the render() in foundation.py. I Would prefer to keep your commit history and contribution rather than making the changes myself.

@rocktavious

This comment has been minimized.

Show comment
Hide comment
@rocktavious

rocktavious Jun 5, 2014

Contributor

will do

Contributor

rocktavious commented Jun 5, 2014

will do

derks added a commit that referenced this pull request Jul 9, 2014

Merge pull request #257 from mapmyfitness/new_feature
app.run should be able to handle the output from controller commands

@derks derks merged commit 9f9a886 into datafolklabs:master Jul 9, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@rocktavious rocktavious deleted the mapmyfitness:new_feature branch Sep 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment