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 Jennifer Recipe #20

Closed
wants to merge 2 commits into from
Closed

Add Jennifer Recipe #20

wants to merge 2 commits into from

Conversation

faustinoaq
Copy link
Contributor

@faustinoaq faustinoaq commented May 28, 2018

Hi @imdrasil @damianham

This PR tries to add a Jennifer Recipe to replace amberframework/amber#766

This will be easier to maintain, contribute, test and deploy because amber templates a becoming too complex, so recipes to rescue!!! 😉

This is an initial implementation, so, there are some issues to fix:

  1. Recipes doesn't support migrations or auth yet (only app, controllers, models, and scaffold): see: https://github.com/amberframework/amber/blob/master/src/amber/cli/recipes/recipe.cr#L25
  2. Jennifer (and some of its dependencies) has wrong tags: see v0.5.1 always fetches dependencies imdrasil/jennifer.cr#137 (this causes broken dependencies, see: Dependencies are satisfied rarely works crystal-lang/shards#183)
  3. amber new doesn't allow other models, just granite and crecto, see: Parsing Error: The -m option must be one of granite, crecto. amber#827
  4. Jennifer doesn't support SQLite yet, see: https://github.com/imdrasil/jennifer.cr#important-restrictions

Roadmap

  • Add jennifer_spec.cr and auto-deploy support using Travis
  • Add Jennifer recipe README
  • Add main app folder with Jennifer dependencies and sam.cr file
  • Add models using Jennifer and model specs
  • Complete scaffolding (views and controllers) using Jennifer and scaffolding specs WIP ⚠️
  • Add auth using Jennifer and auth specs
  • Fix formatting and other specs, see Fixes spec and formatting #15

@imdrasil
Copy link

@faustinoaq after merging master into this branch it is impossible to find what are the original changes. Would you mind if I reset last merge commit and do rebase instead of merge (maybe I will have to squash commits)? The opposite solution is to create separate PR with my original changes and add changes what are done here. What do you think?

@faustinoaq
Copy link
Contributor Author

faustinoaq commented May 28, 2018

@imdrasil Oh, my bad, I just messed up the code, I tried to fix it unsuccessfully, can you create another PR with Jennifer changes? 😅

Edit: Fixed!!! 🎉

@faustinoaq faustinoaq added the status:on-hold on hold - do not merge label May 28, 2018
@faustinoaq
Copy link
Contributor Author

faustinoaq commented May 28, 2018

@imdrasil I'm just wondering why #4 didn't have this merging issue, @damianham WDYT? 😅

Edit: Fixed!!! 🎉

@faustinoaq faustinoaq moved this from To Do to In progress in Framework 2018 May 28, 2018
@faustinoaq
Copy link
Contributor Author

faustinoaq commented May 29, 2018

@imdrasil I just fixed merge issues, All Jennifer code has been synced with master successfully ✨ 🎉

@faustinoaq faustinoaq removed the status:on-hold on hold - do not merge label May 29, 2018
@@ -0,0 +1,32 @@
{% assign id_field_type = "Primary32" -%}

Choose a reason for hiding this comment

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

ATM Jennifer by default use Int32 field database type for a primary key.

{%- for field in fields -%}
{%- if field.name != "id" -%}
{%- if field.type == "reference" %}
{{ field.name }}_id: { type: Int32? },

Choose a reason for hiding this comment

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

Same with foreign keys ^^^

end

def {{ name }}_params
{{ class_name }}.build_params(

Choose a reason for hiding this comment

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

Here I need some advice: Jennifer has own built-in validation + she can't use Hash(String, String) as an argument for building objects. Should I use arguments validation anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

The validation in the controller is validation of the incoming parameters before it hits the ORM layer so I think you would leave that method as it is.

@faustinoaq
Copy link
Contributor Author

Hi @imdrasil Thank you for your latest commits, very appreciated! 👍

Just a comment:

The {{" "}} code is required to preserve white space because liquid templates removes all blank spaces when {%- -%} is used.

Current specs are failing because formatting issues on generated code:

Error: formatting 'src/controllers/category_controller.cr' produced changes
Error: formatting 'src/controllers/product_controller.cr' produced changes
Error: formatting 'src/controllers/comment_controller.cr' produced changes
Error: formatting 'src/models/category.cr' produced changes
Error: formatting 'src/models/product.cr' produced changes
Error: formatting 'src/models/comment.cr' produced changes
Error: 'config/initializers/database.cr' has syntax errors
Error: formatting 'spec/controllers/category_controller_spec.cr' produced changes
Error: formatting 'spec/controllers/product_controller_spec.cr' produced changes
Error: formatting 'spec/controllers/comment_controller_spec.cr' produced changes
Error: formatting 'spec/models/category_spec.cr' produced changes
Error: formatting 'spec/models/product_spec.cr' produced changes
Error: formatting 'spec/models/comment_spec.cr' produced changes

^^ These don't include .ecr nor .slang generated templates, so those should be checked manually 😅

@eliasjpr
Copy link

Ping! @imdrasil @damianham

@imdrasil
Copy link

Hi. I had some stuff to do. Will return to this from Sunday.

@eliasjpr
Copy link

@imdrasil awesome I saw the new version of Jennifer and it would be great to have this recipe available for the next release

@imdrasil imdrasil force-pushed the fa/jennifer-recipe branch 2 times, most recently from aa4c432 to 915acdf Compare September 16, 2018 14:28
@imdrasil imdrasil force-pushed the fa/jennifer-recipe branch 17 times, most recently from 7700faa to 24b225f Compare September 18, 2018 19:40
@@ -7,8 +7,7 @@ logging:
filter:
- password
- confirm_password
skip:
-
skip: []

Choose a reason for hiding this comment

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

such kind of change is made because the previous case created [nil] array.

@imdrasil
Copy link

@faustinoaq , @eliasjpr please take a look at my changes when have some free time

@eliasjpr
Copy link

This is awesome @imdrasil. Can you add a readme to the recipe on how to setup. I think it will come in handy.

@amberframework amberframework deleted a comment from imdrasil Sep 24, 2018
@imdrasil
Copy link

@faustinoaq please take a look at the changes when have some free time

@imdrasil imdrasil removed the pr:wip label Sep 24, 2018
@faustinoaq
Copy link
Contributor Author

@imdrasil Yeah, sorry for late reply, I'm organizing a lot of things first. I'll check this, Don't worry 😉

@robacarp robacarp closed this Jun 17, 2020
Framework 2018 automation moved this from In progress to Done Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants