Have generators handle multi-word names (#316) by marksiemers · Pull Request #330 · amberframework/amber

marksiemers · 2017-10-26T08:00:51Z

Description of the Change

This changes how the generators handle file names and class names. It is related to #316 .
That issue was originally about controllers, but it applies to all the generators, so this fix addresses all of them.

I ended up added a lot of LOC to the test suite. Part of this is due to the before_each and after_each hooks in crystal being global (so I couldn't use them here), another reason is that the generators create a lot of files, checking each one takes a bit of code.

That said, I'm opening this PR for initial feedback to see if this approach is acceptable - both to testing and coding this fix. If there is a better way to structure the test suite, I'm all ears.

Alternate Designs

I thought about DRY'ing the code up a bit so that camelcase wasn't used in so many places, but it would have required adding at least an instance variable and a getter to the Template - in the end that may be the best solution, in case the generators need to deal with other naming considerations (singular and plural).

I went with this solution to get something working, and provided a (hopefully) comprehensive set of tests if a refactor happens in the future.

Benefits

It fixes issue #316

Possible Drawbacks

This change touches a lot of files and has an effect on every generator. Based on the test suite, everything is still generating as normal, but it is possible something breaks in the actual generation of files.

On my machine, one of the pre-existing tests is either not passing, or hanging indefinitely. I checked out the master branch and got the same results.

It hangs here: https://github.com/amberframework/amber/blob/master/spec/amber/cli/commands/generator_spec.cr#L14

Or fails here: https://github.com/amberframework/amber/blob/master/spec/amber/cli/commands/generator_spec.cr#L16

veelenga · 2017-10-26T09:10:08Z

+            File.delete("./src/views/#{snake_case}/edit.slang")
+            File.delete("./src/views/#{snake_case}/index.slang")
+            File.delete("./src/views/#{snake_case}/new.slang")
+            File.delete("./src/views/#{snake_case}/show.slang")


If one expectation above fails these files will not be deleted. Or absence of this files is guaranteed by Amber::CLI::Spec.cleanup ? If not, you may need to use begin ... ensure block for this:

begin # expectations ensure # delete files end

But if it is guaranteed, you don't need to delete them manually.

The calls to Amber::CLI::Spec.cleanup should ensure cleanup, but admittedly, I didn't explicitly test for or look for it.

I did run a bunch of failing test suites though, and there were never any leftovers (according to git).

The entire spec in this file is wrapped in a begin ... ensure ... end that executes Amber::CLI::Spec.cleanup which is more comprehensive than my file deletion.

I can rerun some failing tests and explicitly look for leftover files (as they may be gitignored).

I made it fail one of the should tests on generating a scaffold, then did an ls tmp and it was empty. It looks like we are good to count on Amber::CLI::Spec.cleanup

So you don't need to delete it manually, right ?

When the test suite is complete, Amber::CLI::Spec.cleanup makes sure everything is cleaned up.

The test itself tests two cases of possible CLI input (in a loop) - PostComment and post_comment

The reason for the manual delete is to avoid false green tests with the second iteration of the loop.

Ah, sorry, i haven't noticed the loop above. Thanks for the explanation!

marksiemers · 2017-10-27T16:28:20Z

What's the preferred course of action here? Resolve the conflicts here, or rebase (off master) my changes locally and push with the new commits?

eliasjpr · 2017-10-27T16:48:54Z

Rebase off master

marksiemers · 2017-10-27T22:59:05Z

Rebased, refactored, and add a new concept of display_name
LMKWYT

marksiemers · 2017-10-27T23:52:15Z

Hold off on this, I found a bug while manually testing - I didn't touch the routes generator.

…ramework#316)

…#316)

…mework#316)

…hod (amberframework#316)

…rk#316)

…mberframework#316)

marksiemers · 2017-10-28T10:55:45Z

Manually tested with both Crecto and Granite, and added spec tests for both as well.

LMK if anything else is needed for this PR.

drujensen

lgtm. Nice work. Love all the test coverage. 💯 🥇

drujensen · 2017-10-28T16:08:28Z

@marksiemers I found a couple issues testing the branch:

The name of the project having dashes:

amber new my-cool-blog
cd my-cool-blog
amber g scaffold post title:string
amber db drop create migrate

produces:
syntax error at or near "-"

The config/database.yml needs to be changes to replace the dashes in the db name.

pg:
  database: postgres://root:@localhost:5432/my-cool-blog_development

The field name having dashes:

amber g scaffold post my-title:string

An error ocurred executing migration 20171028085330. Error message is: syntax error at or near "-"

marksiemers · 2017-10-28T17:43:52Z

@drujensen - The issue with the app name having dashes is a pre-existing bug
Reported here: #329
Pending fix here: #332

marksiemers · 2017-10-28T22:15:17Z

@drujensen @veelenga
Any chance of this getting merged soon? The work I'm doing on #294 requires code that will need to be reworked after this change is incorporated.

elorest

Overall I I like this. Don't like all the seemingly repetitive code in generator_spec though.

elorest · 2017-10-30T01:18:51Z

+            File.read("./config/routes.cr").should contain "#{camel_case}Controller"
+            File.read("./config/routes.cr").should_not contain "#{incorrect_case}Controller"
+
+            File.delete("./spec/models/#{snake_case}_spec.cr")


Why are we deleting all of these one by one? We already delete he folder after the test. At least we should be.

Looks like you sort of answered my question below. Even if you do have a reason for deleting these we shouldn't be deleting them one by one.

I can clean it up, the two (not mutually exclusive) ideas that come to mind first are:

Use rm -rf ./spec/* rm -rf ./src/*

Refactor to put the file handling in a module

I can clean this up, two thoughts come to mind (not mutually exclusive), LMKWYT:

Use rm -rf ./src/* rm -rf ./spec/*

Have a module take care of the deletion of files (and potentially other file handling operations)

The first option seems clean and easy to read.

For the scaffold, I changed it so that a new app is created for each iteration of the loop, and the spec cleanup is also run. This is because the routes file is touched and that can't be deleted safely without generating a new app.

When applicable, I switched to rm -rf ./[dir]/*

veelenga

I haven't tried to run generators and test it locally, but the change looks good to me 👍

elorest

Still approved! Thanks.

* Add tests for generating file names and class names correctly (#316) * No longer downcase name when instantiating a Template (#316) * Switch all cases of `@name.capitalize` to `@name.camelcase` (#316) * Switch from `@name.camelcase` everywhere to `class_name` instance method (#316) * Add `display_name` method and use it in generators * Incorporate class naming convention in route generators (#316) * Incorporate table naming convention in crecto and granite generators (#316) * Delete files in a better way in generator_spec

* Add tests for generating file names and class names correctly (#316) * No longer downcase name when instantiating a Template (#316) * Switch all cases of `@name.capitalize` to `@name.camelcase` (#316) * Switch from `@name.camelcase` everywhere to `class_name` instance method (#316) * Add `display_name` method and use it in generators * Incorporate class naming convention in route generators (#316) * Incorporate table naming convention in crecto and granite generators (#316) * Delete files in a better way in generator_spec Former-commit-id: 7e422db

veelenga reviewed Oct 26, 2017

View reviewed changes

marksiemers mentioned this pull request Oct 26, 2017

Controller generator is not great with multi-word names #316

Closed

marksiemers force-pushed the fix-name-generator branch from 45ca56a to 2e43481 Compare October 27, 2017 22:01

marksiemers force-pushed the fix-name-generator branch from 30ab05d to 3ca2585 Compare October 27, 2017 23:29

marksiemers added 6 commits October 27, 2017 23:57

Add tests for generating file names and class names correctly (amberf…

19b57d5

…ramework#316)

No longer downcase name when instantiating a Template (amberframework…

9e3e39f

…#316)

Switch all cases of @name.capitalize to @name.camelcase (amberfra…

16ec349

…mework#316)

Switch from @name.camelcase everywhere to class_name instance met…

1cfd803

…hod (amberframework#316)

Add display_name method and use it in generators

dbba51d

Incorporate class naming convention in route generators (amberframewo…

fc578c0

…rk#316)

marksiemers force-pushed the fix-name-generator branch from 8e01848 to cb4a02e Compare October 28, 2017 10:39

Incorporate table naming convention in crecto and granite generators (a…

cb4a02e

…mberframework#316)

drujensen approved these changes Oct 28, 2017

View reviewed changes

elorest approved these changes Oct 30, 2017

View reviewed changes

veelenga approved these changes Oct 30, 2017

View reviewed changes

Delete files in a better way in generator_spec

911adee

marksiemers mentioned this pull request Oct 30, 2017

Error pages generator #294

Closed

elorest reviewed Oct 30, 2017

View reviewed changes

drujensen merged commit 33a6eb9 into amberframework:master Oct 30, 2017

marksiemers deleted the fix-name-generator branch October 31, 2017 01:23

marksiemers mentioned this pull request Oct 31, 2017

Fix postgresql database naming convention #332

Merged

elorest mentioned this pull request Nov 12, 2017

Document merged PRs from the last month. #384

Closed

40 tasks

elorest mentioned this pull request Nov 12, 2017

Pull Requests which need to be documented before release. #385

Closed

41 tasks

Uh oh!

Conversation

marksiemers commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Uh oh!

veelenga Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veelenga Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marksiemers commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eliasjpr commented Oct 27, 2017

Uh oh!

marksiemers commented Oct 27, 2017

Uh oh!

marksiemers commented Oct 27, 2017

Uh oh!

marksiemers commented Oct 28, 2017

Uh oh!

drujensen left a comment

Choose a reason for hiding this comment

Uh oh!

drujensen commented Oct 28, 2017

Uh oh!

marksiemers commented Oct 28, 2017

Uh oh!

marksiemers commented Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elorest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veelenga left a comment

Choose a reason for hiding this comment

Uh oh!

elorest left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marksiemers commented Oct 26, 2017 •

edited

Loading

veelenga Oct 26, 2017 •

edited

Loading

veelenga Oct 26, 2017 •

edited

Loading

marksiemers commented Oct 27, 2017 •

edited

Loading

marksiemers commented Oct 28, 2017 •

edited

Loading