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

Add guides around code organization and file formatting #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blumhardts
Copy link
Contributor

FMS is a large code base without much consistency in how its organized on a file level. Models, controllers, and tests all have different code groupings which this commit seeks to standardize.

This is just a rough first draft. I realize code organization is probably a contentious topic for some so I'd like as much feedback as possible.

The end goal, and what @adamnengland and I discussed, is to create some checks in Pronto to ensure files are organized according to the standards we all agree on here.

This style guide is great, but it's a passive resource. If we can get more checks for things like this (and commit messages, which I've discussed a bit with @thetimbanks) I think we'd see a marked increase in our code quality.

FMS is a large code base without much consistency in how its organized
on a file level. Models, controllers, and tests all have different
code groupings which this commit seeks to standardize.
Copy link

@adamnengland adamnengland left a comment

Choose a reason for hiding this comment

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

@blumhardt Nice overall, just a couple small points where I don't agree.

end
```

### Model File Organization

Choose a reason for hiding this comment

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

Where do non-default scopes go in this setup? I'd suggest they go right after the associations, but I'm not committed to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've always put them before public class methods, since they're more or less just chainable class methods.


```ruby
describe User do
describe('Default Scope') do

Choose a reason for hiding this comment

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

I'm a believer that scopes should not be tested (at least not a a unit test level), so I would leave this example out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to agree and gets me thinking that there should probably be a testing do's and dont's page in this guide too. @bgibbons0531 said this in a thread in Slack on 01-22-2018:

Would make it more... friendly... to add tests as a visiting dev too. When I add things to FMS, I figure I should add a test around it (even if super small), look through test code base, and then think to myself maybe it's not that important

Even just a list of things that must be tested before opening a pull request because I was on the same page with him in that I didn't know which tests I should include in my first PR.

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