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

Support namespaces in model generator #1588

Merged

Conversation

NotWearingPants
Copy link
Contributor

@NotWearingPants NotWearingPants commented Oct 2, 2021

Purpose

Fixes #1184
Adds support for generating namespaced models using the model generator.

Description

Previously:

$ lucky gen.model Blog::Post title:String
Model name should only contain letters. Example: lucky gen.model BlogPost

But now it will create src/models/blog/post.cr with the class Blog::Post.

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

I've worked on this using the GitHub web IDE so I never compiled/ran the code, but I've tried real hard to make sure everything is in order (even read the source code of teeplate and some avram), so I'm pretty confident this works. If not I'll fix it.

btw I've never heard/used the Crystal language until now, and I hate it. But hopefully I helped. Although the macro feature is impressive.

Note

There's a small edge case - all of Foo::Bar::Baz/Foo::BarBaz/FooBar::Baz/FooBarBaz use create_foo_bar_baz for the name of the migration Avram creates, so if you try to create this conflict then the generation will succeed but the migration generation will fail with Migration name must be unique.

I don't think this is fixable without changing Avram, since it expects a camel-cased name and converts it to underscored, so even if we wanted Blog::Post to turn into create_blog__post (double underscore to mark namespace) it isn't possible.
And I can't check for this conflict easily since I'd have to check all possibilities of adding double-colons between every word. Or I'd have to look in the migrations folder which is easier, but breaks the abstraction.

@jwoertink
Copy link
Member

Thanks for the contribution! I know this codebase isn't easy to work with when you're used to, so jumping in with no knowledge is pretty amazing. The tricky part here is, there's no good way to test this without actually using it in an App, so I'll have to take a bit of time to test this out before I can merge it in. I'll try to get that done today though.

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Alright, I've pull this branch in to a fresh app and tested it out.

  • Generated a new app
  • Generated a regular model Post
  • Generated a namespace model Bar::Tender
  • Generated a nested namespace model Admin::Foo::Ad
  • Generated a browser resource (to ensure nothing changed there)

I did find some issues, but completely unrelated to this PR. I know this wasn't exactly your favorite to work on, but I really appreciate you taking the time to do this. It helps a ton!

@jwoertink jwoertink added the hacktoberfest-accepted PRs accepted for Hacktoberfest label Oct 2, 2021
@jwoertink jwoertink merged commit b6f68bd into luckyframework:master Oct 2, 2021
@wout
Copy link
Contributor

wout commented Oct 3, 2021

This is fab! Can't wait to get my hands on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted PRs accepted for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support namespaces on model generator
3 participants