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 spring-controller sub-generator #6451

Merged
merged 10 commits into from Oct 11, 2017

Conversation

Projects
None yet
4 participants
@xetys
Member

xetys commented Oct 2, 2017

for #6440

based on service and renamed service subgen to spring-service

I would like to add both, travis tests and integration tests for the controller...

additionally, it might be interesting for the interactive mode to add controller actions like fields in entity generator, where the user can choose the HTTP method and the action's name. WDYT?

  • Travis tests are green
  • Tests are added where necessary
  • Documentation is added/updated where necessary
  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed
created basic controller sub-generator based on service
and renamed service subgen to spring-service
Show outdated Hide outdated cli/commands.js
@xetys

This comment has been minimized.

Show comment
Hide comment
@xetys

xetys Oct 2, 2017

Member
Member

xetys commented Oct 2, 2017

@MathieuAA

This comment has been minimized.

Show comment
Hide comment
@MathieuAA

MathieuAA Oct 2, 2017

Member

@xetys that's the trade-off =/ it's generally the linter that warns you (more of an error-warning :)) about it.
It seems, in our case, that the tests fail because of that.

Member

MathieuAA commented Oct 2, 2017

@xetys that's the trade-off =/ it's generally the linter that warns you (more of an error-warning :)) about it.
It seems, in our case, that the tests fail because of that.

Show outdated Hide outdated cli/commands.js
@xetys

This comment has been minimized.

Show comment
Hide comment
@xetys

xetys Oct 2, 2017

Member

@deepu105 seriously...ok... I will change that

Member

xetys commented Oct 2, 2017

@deepu105 seriously...ok... I will change that

public class <%= controllerClass %>Controller {
private final Logger log = LoggerFactory.getLogger(<%= controllerClass %>Controller.class);

This comment has been minimized.

@jdubois

jdubois Oct 3, 2017

Member

Maybe we should also add a simple "GET" method? Like a "helloworld()" method that returns a String?

@jdubois

jdubois Oct 3, 2017

Member

Maybe we should also add a simple "GET" method? Like a "helloworld()" method that returns a String?

This comment has been minimized.

@xetys

xetys Oct 3, 2017

Member

How about a similar process like the field adding in entity sub-generator? And if no method added, then add a hello-world method

@xetys

xetys Oct 3, 2017

Member

How about a similar process like the field adding in entity sub-generator? And if no method added, then add a hello-world method

This comment has been minimized.

@deepu105

deepu105 Oct 3, 2017

Member

That would be much better otherwise the class we generate wouldnt have much value over the one you could create in an IDE

@deepu105

deepu105 Oct 3, 2017

Member

That would be much better otherwise the class we generate wouldnt have much value over the one you could create in an IDE

This comment has been minimized.

@jdubois

jdubois Oct 3, 2017

Member

yes +1 - but keep the question simple

@jdubois

jdubois Oct 3, 2017

Member

yes +1 - but keep the question simple

* <%= controllerClass %> controller
*/
@RestController
@RequestMapping("/api/<%= apiPrefix %>")

This comment has been minimized.

@jdubois

jdubois Oct 3, 2017

Member

Maybe we should also generate a unique URL? Here you would get error when you generate 2 controllers in a row. How about "/api/<%= apiPrefix %>-<%=controllerClass%>"

@jdubois

jdubois Oct 3, 2017

Member

Maybe we should also generate a unique URL? Here you would get error when you generate 2 controllers in a row. How about "/api/<%= apiPrefix %>-<%=controllerClass%>"

This comment has been minimized.

@jdubois

jdubois Oct 3, 2017

Member

Maybe use Lodash to use a dasherized version of "controllerClass"

@jdubois

jdubois Oct 3, 2017

Member

Maybe use Lodash to use a dasherized version of "controllerClass"

This comment has been minimized.

@xetys

xetys Oct 3, 2017

Member

apiPrefix = kebabCase(controllerClass)

@xetys

xetys Oct 3, 2017

Member

apiPrefix = kebabCase(controllerClass)

This comment has been minimized.

@jdubois

jdubois Oct 3, 2017

Member

the issue with KebabCase is that you would keep upper case letters -> maybe do a lower case after?

@jdubois

jdubois Oct 3, 2017

Member

the issue with KebabCase is that you would keep upper case letters -> maybe do a lower case after?

This comment has been minimized.

@xetys

xetys Oct 3, 2017

Member

no, it doesn't, I tried already...

/**
 * AnotherTest controller
 */
@RestController
@RequestMapping("/api/another-test")
public class AnotherTestController {
    //...
}
@xetys

xetys Oct 3, 2017

Member

no, it doesn't, I tried already...

/**
 * AnotherTest controller
 */
@RestController
@RequestMapping("/api/another-test")
public class AnotherTestController {
    //...
}

This comment has been minimized.

@jdubois

jdubois Oct 4, 2017

Member

I'm quite surprised, good news then, let's do it!

@jdubois

jdubois Oct 4, 2017

Member

I'm quite surprised, good news then, let's do it!

@xetys

This comment has been minimized.

Show comment
Hide comment
@xetys

xetys Oct 3, 2017

Member

my todos left:

  • yeoman tests
  • Spring integration tests
  • (in discusson) method generator
  • default action (if no other action exists)
Member

xetys commented Oct 3, 2017

my todos left:

  • yeoman tests
  • Spring integration tests
  • (in discusson) method generator
  • default action (if no other action exists)
@xetys

This comment has been minimized.

Show comment
Hide comment
@xetys

xetys Oct 6, 2017

Member

so far, here it is

Member

xetys commented Oct 6, 2017

so far, here it is

@xetys xetys added needs-review and removed work-in-progress labels Oct 6, 2017

@xetys xetys requested review from jdubois and deepu105 Oct 6, 2017

* @see <%= controllerClass %>Controller
*/
@RunWith(SpringRunner.class)
@SpringBootTest(classes = <%= mainClass %>.class)

This comment has been minimized.

@jdubois

jdubois Oct 6, 2017

Member

Oh you even generate tests on your controller!!! Big +1!

@jdubois

jdubois Oct 6, 2017

Member

Oh you even generate tests on your controller!!! Big +1!

@jdubois

There are a few comments (mostly from @deepu105 ) but once you solve them, we're good to go! Great job!

return 'Your action name cannot be empty';
} else if (input.charAt(0) === input.charAt(0).toUpperCase()) {
return 'Your action name cannot start with an upper case letter';
} else if (jhiCore.isReservedFieldName(input)) {

This comment has been minimized.

@deepu105

deepu105 Oct 11, 2017

Member

@xetys does this matter? I guess reserved words are ok in a method name??

@deepu105

deepu105 Oct 11, 2017

Member

@xetys does this matter? I guess reserved words are ok in a method name??

This comment has been minimized.

@deepu105

deepu105 Oct 11, 2017

Member

Ok never mind seems some keywords cant be used anyway

@deepu105

deepu105 Oct 11, 2017

Member

Ok never mind seems some keywords cant be used anyway

@deepu105

I'll see if i can fix the one remaining issue

this.controllerActions.push(controllerAction);
askForControllerAction.call(this, done);

This comment has been minimized.

@deepu105

deepu105 Oct 11, 2017

Member

you cant do this for arrow methods just call askForControllerAction(done)

@deepu105

deepu105 Oct 11, 2017

Member

you cant do this for arrow methods just call askForControllerAction(done)

@deepu105 deepu105 merged commit 3f02ca0 into jhipster:master Oct 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@deepu105 deepu105 referenced this pull request Oct 11, 2017

Closed

Renamed the service sub-generator to spring-service #6502

2 of 2 tasks complete

@jdubois jdubois added this to the 4.10.0 milestone Oct 17, 2017

@jdubois

This comment has been minimized.

Show comment
Hide comment
@jdubois

jdubois Oct 17, 2017

Member

I'm rushing to do the 4.10.0 release, so sorry I'm going to modify this without much discussion:

  • This generates a "FooController" class, but the entity sub-generator would generate a "FooResource" class. This is because that's supposed to be a REST "resource". Anyway, for consistency, I'm going to rename this with the "Resource" prefix.
  • I'm also going to add the documentation to the website.
Member

jdubois commented Oct 17, 2017

I'm rushing to do the 4.10.0 release, so sorry I'm going to modify this without much discussion:

  • This generates a "FooController" class, but the entity sub-generator would generate a "FooResource" class. This is because that's supposed to be a REST "resource". Anyway, for consistency, I'm going to rename this with the "Resource" prefix.
  • I'm also going to add the documentation to the website.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment