-
-
Notifications
You must be signed in to change notification settings - Fork 970
7.1.x cli add-field and add-property for domain field and property additions #15289
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
base: 7.1.x
Are you sure you want to change the base?
Conversation
… the command line
|
|
||
| implementation project(':grails-forge-core') | ||
| implementation "org.apache.grails.bootstrap:grails-bootstrap:$projectVersion" | ||
| implementation "org.codehaus.groovy:groovy:$groovyVersion" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Groovy 4.x is org.apache.groovy! This is polluting the classpath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in the forge-cli, which is still a micronaut app and on groovy 3.
jdaugherty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only concern I have with this PR is the decision to put it in grails-bootstrap. I am assuming end user apps won't be generating fields at runtime of a Grails application so this is the wrong spot to place it.
grails-bootstrap/src/main/groovy/grails/codegen/model/DomainFieldModifier.groovy
Show resolved
Hide resolved
grails-bootstrap/src/main/groovy/grails/codegen/model/DomainFieldModifier.groovy
Outdated
Show resolved
Hide resolved
grails-bootstrap/src/main/groovy/grails/codegen/model/DomainFieldModifier.groovy
Outdated
Show resolved
Hide resolved
grails-bootstrap/src/main/groovy/grails/codegen/model/DomainFieldModifier.groovy
Outdated
Show resolved
Hide resolved
| } else { | ||
| int constraintBlockIndex = fieldInsertIndex + 1 | ||
| lines.add(constraintBlockIndex, '') | ||
| lines.add(constraintBlockIndex + 1, ' static constraints = {') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't formatting be handled independently of line addition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you suggesting to reformat the entire file after making changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to suggest extracting the spacing instead of hard coding the spaces into the argument.
i.e.
int indent = 5
"${indent*1}static constraints = {"
This way you're not hard coding various length spaces and later if we have to change the indent / wrap the code, we don't have to manually modify every line.
grails-bootstrap/src/main/groovy/grails/codegen/model/DomainFieldModifier.groovy
Show resolved
Hide resolved
grails-bootstrap/src/main/groovy/grails/codegen/model/FieldDefinition.groovy
Outdated
Show resolved
Hide resolved
|
|
||
| implementation project(':grails-forge-core') | ||
| implementation "org.apache.grails.bootstrap:grails-bootstrap:$projectVersion" | ||
| implementation "org.codehaus.groovy:groovy:$groovyVersion" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in the forge-cli, which is still a micronaut app and on groovy 3.
|
This looks OK to me. (We could move code generation classes to it's own module in Grails 8 if we want to get them out of |
@matrei I modeled it after Spring Roo Is the Groovy consensus still that it is a property because it has no access modifier? This is from 2014: |
@codeconsole |
ok, that is pretty clear. Thank you. Since there is no access modifier, I agree that we should use add-property for property additions. Since add-field could also be useful, I think we should have add-field for fields and add-property for properties. add-field now requires passing an access modifier. |
@codeconsole What is the use case for adding a field to a domain class, and does that use case warrant a shell command? |
@matrei well, it's kind of a freebie due to inheritance. The original premise behind this PR was to slowly introduce codegen capabilities to where you could build a workable demo with cli commands. I've had use cases for static fields in my domain objects. I suppose I am indifferent. Adding properties is what is most important. Should we get rid of add-field altogether? |
@codeconsole My preference would be to remove |
|
@matrei ok, I will get rid of it. |
add-property CLI Command
Overview
The add-property command adds a new property to an existing Grails domain class, optionally with validation
constraints using either Grails constraints or Jakarta Validation annotations.
Usage
grails add-property FIELD:TYPE [OPTIONS]
Arguments
Supported Field Types
Options
@NotNullor nullable: false)@NotBlankor blank: false)Constraint Styles
@NotNull,@NotBlank,@Size)Examples
Basic field (no constraints)
add-field Book title:String
Result:
With Grails constraints (default)
add-field Book title:String --not-nullable --max-size 255
Result:
With Jakarta Validation annotations
add-field Book title:String --not-nullable --not-blank --max-size 255 --constraint-style jakarta
Result:
With both constraint styles
add-field Book title:String --not-nullable --max-size 255 --constraint-style both
Result:
Constraint Mapping
Error Handling