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

Cop that checks class structure #1575

Closed
bbatsov opened this issue Jan 17, 2015 · 12 comments
Closed

Cop that checks class structure #1575

bbatsov opened this issue Jan 17, 2015 · 12 comments

Comments

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 17, 2015

This is probably one of the most important rules in the style guide that lacks a cop.

P.S. We should probably create a list of rules that we're not currently enforcing.

@d4rk5eed
Copy link

+1
What about duplicate methods?

class A
  def method
     #legacy implementation
  end
  def method
     #new implementation
  end
end

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jan 17, 2015

@d4rk5eed Yeah, such a cop would be useful (and very easy to implement).

@alexdowad
Copy link
Contributor

I don't like the "enforced class structure". I prefer to group methods/constants/etc. according to what they are used for, not put them in a rigid order which doesn't reflect their meaning.

The "duplicate method" cop is a good idea; I'll add one right now.

@alexdowad
Copy link
Contributor

OK, it turns out we already had a "duplicate method" cop... but I just gave it a major upgrade. Have a look at the open PR.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Dec 22, 2015

I don't like the "enforced class structure". I prefer to group methods/constants/etc. according to what they are used for, not put them in a rigid order which doesn't reflect their meaning.

I don't think the order conveys that much meaning. I've been applying exactly the same structure in my classes for as long as I can remember and this makes it really easy for me to get my bearings. For me this is one of the most important cops that's still missing.

@alexdowad
Copy link
Contributor

I've implemented a Style/ClassStructure cop.

One thing I have found is that the rule about nested classes is a prime source of false positives. Many classes may have a "helper" class which is tucked down at the bottom of the class body. The style guide's order forces these nested classes to be pulled up to the top, above the methods which actually use them.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Dec 29, 2015

Well, this can always be made configurable.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jan 1, 2016

@alexdowad Btw, what happened to this cop? I thought it was going to get a PR of its own.

@alexdowad
Copy link
Contributor

@bbatsov I'm not interested in working on it further. There are better things to work on. So I haven't opened another PR.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jan 1, 2016

Got it.

@yob
Copy link

yob commented Jan 2, 2016

Even though it'd need more work, is the cop available on branch somewhere? I'd be interested in running it over our code base to see what happens.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jan 2, 2016

It's here - alexdowad@acf7a72

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

No branches or pull requests

5 participants