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

Recognize methods defined with class << self syntax as singleton methods. #539

Conversation

troessner
Copy link
Owner

Fixes #112
The way this works is that we're passing the parent scope for each sexp to Treedresser and then store this parent scope with each adorned ASTNode.
We then check in the UtilityFunction Detector if the parent scope is of type ":sclass".
I believe this is a simple yet elegant solution.

@troessner troessner changed the title Recognize methods defined with class << self syntax as singleton meth… Recognize methods defined with class << self syntax as singleton methods. Jun 14, 2015
@troessner troessner force-pushed the recognize-methods-defined-with-class-self-syntax-as-singleton-methods branch 2 times, most recently from 6926591 to e1eebdf Compare June 14, 2015 19:20
@troessner troessner force-pushed the recognize-methods-defined-with-class-self-syntax-as-singleton-methods branch from e1eebdf to 182018d Compare June 14, 2015 20:45
@mvz
Copy link
Collaborator

mvz commented Jun 15, 2015

I'm of two minds about this solution. On the one hand it is rather elegant in its simplicity, on the other hand it makes the AST data structure more complex, storing the parent in a lot of places where it won't be needed.

Have you considered putting this logic in the CodeContext classes? They already record their parent context. Perhaps adding a SclassContext would work?

@troessner
Copy link
Owner Author

Have you considered putting this logic in the CodeContext classes? They already record their parent context.

Yes, I did consider this.
Let me explain why I think that the above way is a better way than using a CodeContext:

1.) Regarding passing in the parent context: We are traversing the AST with TreeDresser and with TreeWalker. It feels kind of weird and inconsistent that TreeWalker basically allows traversing the tree in both directions, while TreeDresser doesn't.

storing the parent in a lot of places where it won't be needed.

But TreeWalker is the biggest offender in regards to that. If you look at that with a debugger: just a "p self" in a TreeWalker blows up your screen.
I know that the "But this thing is worse" is not the best logic, I'm just mentioning it to keep things in perspective.
And adding just the parent scope is a rather small, sensible addition in my mind that makes sense for TreeDresser.

2.) Regarding the "singleton_method?" in SexpExtensions:
If you look at the current way SexpExtensions work or rather, what kind of functionality is encapsulated there, putting the "singleton_method?" there fits right into purpose of it.
We're adorning the tree with the information that a given method is a singleton_method - this feels just right to me.:)

3.) TreeWalker is a very messy already and it has unclear responsibilities (tree walking, reference counting, smell checking). CodeContexts are a little better but adding anything there would still add technical debt. IMHO this whole TreeWalker should be refactored before we add anything to it at all.

Taking all of the above into account, I believe the solution in this pull request is the cleanest way.

@troessner
Copy link
Owner Author

Pinging @mvz - I believe I presented a couple of good arguments for this solution, so if you don't have any serious showstoppers I'd appreciate a merge so we can cross this ages-old bug off of our list.;)

@mvz
Copy link
Collaborator

mvz commented Jun 19, 2015

I think this is a good solution for now and we can always revisit it once we start cleaning out TreeWalker.

mvz added a commit that referenced this pull request Jun 19, 2015
…-class-self-syntax-as-singleton-methods

Recognize methods defined with class << self syntax as singleton methods.
@mvz mvz merged commit 4b2290e into develop Jun 19, 2015
@mvz mvz deleted the recognize-methods-defined-with-class-self-syntax-as-singleton-methods branch June 19, 2015 12:11
This was referenced Jun 20, 2015
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.

None yet

2 participants