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

Organize level models inside a levels/ directory #14768

Merged
merged 1 commit into from May 3, 2017

Conversation

joshlory
Copy link
Contributor

@joshlory joshlory commented May 1, 2017

Put all the models for the Level single table inheritance into one folder. Based on the answer to this question: http://stackoverflow.com/questions/18934115/rails-4-organize-rails-models-in-sub-path-without-namespacing-models.

@ashercodeorg
Copy link
Contributor

My vote would be to keep Level (the base class) in the "base" directory. Otherwise, this looks awesome!

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

I love it!

I'd have a mild preference for keeping level.rb in the subdirectory, just to keep tab completion simpler. Asher, why do you prefer it in the base directory?

@ashercodeorg
Copy link
Contributor

Though tab completion simplicity would be nice, it'd also be nice to see the model files for DB tables in the base directory (e.g., there is a levels table, though not a applab table). By no means am I blocking the PR as-is, though.

@joshlory
Copy link
Contributor Author

joshlory commented May 3, 2017

Any further comments here? I'm leaning slightly towards leaving Level in models/levels/level.rb.

@ashercodeorg
Copy link
Contributor

SGTM. And it'd be trivial to move the file later, if we find a compelling reason to do so.

@joshlory joshlory merged commit 5d31278 into staging May 3, 2017
@joshlory joshlory deleted the organize-level-models branch May 3, 2017 17:56
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

4 participants