Remove Method-Level Comments

[tpendragon]

I'm going to propose we remove the requirement of method-level comments. I see this as adding extra development overhead to inform about input/output when the methods should be small enough and properly named to be able to tell what's going on.

This is not to say that we should remove all informative comments - it's okay to leave notes regarding the purpose behind a method or a specific implementation, but they shouldn't be required and rather left as a reminder when they're felt to be necessary.

Thoughts?

[no-reply]

+1

methods should be small enough and properly named to be able to tell what's going on.

It's worth noting that method length is enforced by the Ruby style guidelines and Rubocop.

[jechols]

I'd vote for putting these as a recommendation - maybe not a requirement, but class- and method-level comments can help somebody new a great deal, or in my case, somebody not so new but who hasn't been on the project in a while.

[tpendragon]

I'd vote for putting these as a recommendation - maybe not a requirement, but class- and method-level comments can help somebody new a great deal, or in my case, somebody not so new but who hasn't been on the project in a while.

Can you give me some examples in the OD code base that caused you trouble? It may help to clarify the standards on this issue.

[jechols]

Document class has no comments and no logic - I don't know what we're planning to do with it, if it's dead code, or what.

Documents class doesn't even subclass anything and has no comments or logic.

I'm pretty sure I know what the GenericAsset class is all about, but would somebody new? Class-level comments make a lot of sense here. Same with GenericCollection.

Image class is fairly obvious, but I'd still like to see explanation as to its purpose - it's meant to back specific collections and whatnot. A two-sentence comment wouldn't hurt here.

Documents::Base is just a generic asset with nothing else, but it is Documents::Paged's parent class. Explanations for the purpose of both would be good. What is a paged document? A PDF? Or a document with multiple images in a specific order? I'd have assumed a Document is any generic "thing" like a PDF or Word doc, but I'm not sure how all four Document* classes interact.

As I mentioned before, the Crosswalk stuff isn't obvious, either, but you said those needed to be removed, so hopefully that clears up one issue.

Some of the high-level architecture info may make sense in the top-level README.md or the wiki, but class- and method-level comments are more helpful for a developer.

[tpendragon]

Documents class is a module, it's purely for namespacing. If that's unclear by the structure I can see the purpose to adding some comments.

I see great points here for class-level comments. Perhaps we need to change the documentation to include something regarding explaining the purpose of a class in top-level comments?

[jechols]

Didn't notice that Documents was a module - but it wouldn't hurt to explain it as a container-only module.

I think class comments should be a strong recommendation (maybe not "always", but there should be a good reason not to have class-level comments at the least), and I'd still push for method comments as a recommendation (80/20 kind of thing).

[tpendragon]

I think class comments should be a strong recommendation (maybe not "always", but there should be a good reason not to have class-level comments at the least), and I'd still push for method comments as a recommendation (80/20 kind of thing).

Given this conversation, could you submit a pull request with your suggested changes? Give us something a little more concrete to work the details out on and make sure we all agree.