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 no_empty_functions rule that triggers if an empty function is defined #254

Merged
merged 1 commit into from Apr 16, 2014

Conversation

Projects
None yet
3 participants
@misterdjules
Contributor

misterdjules commented Apr 4, 2014

In the past, I have been bitten by the following code:

someFunctionWithCallback ->
doSomethingSignificant()

What's wrong is that doSomethingSignificant is called no matter what after the call to someFunctionWithCallback, whereas what I really wanted to do is to call doSomethingSignificant from within the callback. It makes a big difference whether someFunctionWithCallback is asynchronous or not.

These kind of issues can sometimes be very tricky to find. This error is often located deep into the source code and when you look at it, it's hard to spot that an indentation token is missing. Moreover, the execution of such code may look perfectly valid, since you expected doSomethingSignificant to be called anyway.

The no_empty_functions rule triggers an error on such code.

I have never used empty functions myself, but I know that other programmers like to do so. When one wants to define an empty function, one only has to use undefined as the only expression within the function body, like so:

someFunction ->
  undefined

It states the intention clearly, and it's not too long to type. It's even similar to Python's pass statement.

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Apr 4, 2014

Contributor

The travis builds fail, I'm working on fixing it.

Contributor

misterdjules commented Apr 4, 2014

The travis builds fail, I'm working on fixing it.

@AsaAyers

This comment has been minimized.

Show comment
Hide comment
@AsaAyers

AsaAyers Apr 4, 2014

Collaborator

I think this demonstrates why this rule should be ignore by default. I think empty functions are more common than you think.

Collaborator

AsaAyers commented Apr 4, 2014

I think this demonstrates why this rule should be ignore by default. I think empty functions are more common than you think.

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Apr 4, 2014

Contributor

@AsaAyers You are right. I set the rule to ignore by default. I hadn't set it to ignore initially because I took another rule with a default level set to warn as a model. I do not think that empty functions are uncommon for everyone. I just think they are very rarely used by some people (including me), and for them they almost always represent a hard to find bug.

Contributor

misterdjules commented Apr 4, 2014

@AsaAyers You are right. I set the rule to ignore by default. I hadn't set it to ignore initially because I took another rule with a default level set to warn as a model. I do not think that empty functions are uncommon for everyone. I just think they are very rarely used by some people (including me), and for them they almost always represent a hard to find bug.

@AsaAyers

This comment has been minimized.

Show comment
Hide comment
@AsaAyers

AsaAyers Apr 10, 2014

Collaborator

Do you/your rule consider this an empty function?

class Foo
    setValue: (@_value) ->
Collaborator

AsaAyers commented Apr 10, 2014

Do you/your rule consider this an empty function?

class Foo
    setValue: (@_value) ->
@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Apr 10, 2014

Contributor

@AsaAyers Yes, it does.

In order not to consider the following function an empty function

class Foo
    setValue: (@_value) ->

I guess I could check if the 'Code' node has a child 'Param' node that matches the following pattern:

Value "this"
    Access whatever

However, the goal is to avoid these forms:

class Foo
    setValue: (@_value) ->
    functionThatDoesSomethingSignificant()

where functionThatDoesSomethingSignificant() should have been indented.

Contributor

misterdjules commented Apr 10, 2014

@AsaAyers Yes, it does.

In order not to consider the following function an empty function

class Foo
    setValue: (@_value) ->

I guess I could check if the 'Code' node has a child 'Param' node that matches the following pattern:

Value "this"
    Access whatever

However, the goal is to avoid these forms:

class Foo
    setValue: (@_value) ->
    functionThatDoesSomethingSignificant()

where functionThatDoesSomethingSignificant() should have been indented.

Show outdated Hide outdated src/rules/no_empty_functions.coffee
@@ -0,0 +1,23 @@
isEmptyCode = (node) ->
node.constructor.name is 'Code' and node.body.isEmpty()

This comment has been minimized.

@zmbush

zmbush Apr 11, 2014

Contributor

Instead of node.constructor.name, use astApi.getNodeName node (See #255)

@zmbush

zmbush Apr 11, 2014

Contributor

Instead of node.constructor.name, use astApi.getNodeName node (See #255)

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Apr 11, 2014

Contributor

@zmbush Thank you for the heads up! I just pushed a change that uses ASTApi.getNodeName instead of node.constructor.name.

Contributor

misterdjules commented Apr 11, 2014

@zmbush Thank you for the heads up! I just pushed a change that uses ASTApi.getNodeName instead of node.constructor.name.

AsaAyers added a commit that referenced this pull request Apr 16, 2014

Merge pull request #254 from misterdjules/no_empty_functions
Add no_empty_functions rule that triggers if an empty function is defined

@AsaAyers AsaAyers merged commit 2987e3c into clutchski:master Apr 16, 2014

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Apr 16, 2014

Contributor

@AsaAyers Thanks for the merge! How is the options documentation generated on http://www.coffeelint.org/ ? In other words can I help to add some documentation about the new no_empty_functions rule when it's released?

Contributor

misterdjules commented Apr 16, 2014

@AsaAyers Thanks for the merge! How is the options documentation generated on http://www.coffeelint.org/ ? In other words can I help to add some documentation about the new no_empty_functions rule when it's released?

@AsaAyers

This comment has been minimized.

Show comment
Hide comment
@AsaAyers

AsaAyers Apr 16, 2014

Collaborator

It's extracted from the descriptions property on the rule. It lives in the gh-pages branch.

Collaborator

AsaAyers commented Apr 16, 2014

It's extracted from the descriptions property on the rule. It lives in the gh-pages branch.

@AsaAyers

This comment has been minimized.

Show comment
Hide comment
@AsaAyers

AsaAyers Apr 16, 2014

Collaborator

I recommend a 2nd pull request to edit the description.

I know it's awkward, but if you want to preview what will be generated for the next release run this series of commands.

# Might need to change master to your branch name.
git checkout master &&
npm run compile &&
git checkout gh-pages &&
cp lib/coffeelint.js js/coffeelint.js &&
rake updatehtml

When I prepare the release I'll update the changelog and run these commands.

Collaborator

AsaAyers commented Apr 16, 2014

I recommend a 2nd pull request to edit the description.

I know it's awkward, but if you want to preview what will be generated for the next release run this series of commands.

# Might need to change master to your branch name.
git checkout master &&
npm run compile &&
git checkout gh-pages &&
cp lib/coffeelint.js js/coffeelint.js &&
rake updatehtml

When I prepare the release I'll update the changelog and run these commands.

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Apr 16, 2014

Contributor

Alright thank you for the info! I'll preview the generated documentation and submit a new pull request if needed.

Contributor

misterdjules commented Apr 16, 2014

Alright thank you for the info! I'll preview the generated documentation and submit a new pull request if needed.

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Apr 17, 2014

Contributor

PR #261 created to better document the no_empty_functions rule.

Contributor

misterdjules commented Apr 17, 2014

PR #261 created to better document the no_empty_functions rule.

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