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

Class name already contained Controller #396

Merged
merged 1 commit into from
Oct 1, 2018

Conversation

vaibhavmule
Copy link
Contributor

Don't need Controller in the doc strings as Class name already has controller in it

Don't need `Controller` in the doc strings as Class name already has `controller` in it
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.699% when pulling e821f3e on vaibhavmule-patch-2 into 35f0cfd on develop.

@josephmancuso josephmancuso merged commit 8f30abf into develop Oct 1, 2018
Copy link
Contributor

@bjorntheart bjorntheart left a comment

Choose a reason for hiding this comment

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

The class name does not necessarily have the word Controller in it. The reason being that you can pass [-e|--exact] to the controller generator command which generate a class name such as Something when you run the command craft controller -e Something. This produces the following code

class Something:

    def show(self):
        pass

When you omit the [-e|--exact] flag it produces the following code

class SomethingController:

    def show(self):
        pass

@josephmancuso
Copy link
Member

@bjorntheart Very good point! My reasoning was that most of the time the user will not use the -e flag and it's just a docstring. People can modify it. But good point that the class won't necessarily always have the controller name in it.

@vaibhavmule vaibhavmule deleted the vaibhavmule-patch-2 branch October 1, 2018 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants