-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Improve generators attributes #5988
Improve generators attributes #5988
Conversation
Structural and general changes - (1) Extract `Mix.Phoenix.Attribute` module. It parses cli attribute into `Attribute` struct, with validation of types and options, and prefilling some default options. Covered with tests. - (2) Create specs for attribute types and options, with examples, to be used in generators docs and in console errors. - (3) Extract `Mix.Phoenix.Migration` module. It prepares data, based on `Attribute` struct, to apply in migration file. Covered with tests. This eliminate migration data preparation in cases with no migration generation (e.g. for embedded schema or when flag is passed). Thoughts for later: potentially this logic can be used in new `mix phx.gen.migration`, as extension of `mix echo.gen.migration` with attributes. - (4) Extract `Mix.Phoenix.TestData` module. It prepares data, based on `Attribute` struct, to apply in test files. Covered with tests. - (5) Refactor only schema related data preparation in `Mix.Phoenix.Schema`, based on `Attribute` struct. Only parsing cli attributes and preparing general `sample_values` is done during creation of `Schema` struct. Specific data for generated files are created on demand based on `schema.attrs` and `schema.sample_values`. - (6) Extracted `Mix.Phoenix.Web` module. It prepares data, based on `Attribute` struct, to apply in html, live files. Covered with tests. It also postpone data generation. Thus e.g. data generation will be invoked only if conflicts check passes. - (7) Add utility function `Mix.Phoenix.indent_text` to indent text or list of lines with spaces, and prepend and append empty lines when needed. Use it for new and old refactored cases. - (8) Generate `context` files before `json`, `html`, `live` files, as it doesn't need specific extra bindings. - (9) Extract fixture data generation, to be done only when fixture is going to be created. - (10) Extract html assertion data generation, to be done only when web files is going to be created. - (11) Correct array values rendering for index and html. Array of integers is interpolating into string with wrong representation (`"#{[1,2]}"` is `<<1, 2>>`, `"#{[42,43]}"` is `"*+"`). And it even leads to errors during run of generated tests (e.g. `"#{[1042,1043]}"` leads to error `(ArgumentError) lists in Phoenix.HTML templates only support iodata, and not chardata.`). Correct with simple default rendering logic - adjustment with `(values || []) |> List.flatten() |> Enum.join(", ")`. It works for any levels nesting arrays. And feels reasonable default representation for array values. - (12) In generated tests files, relocate `create_attrs` and `update_attrs` from module attributes into `test` body. More practical approach, as more often we need to generate some data. E.g. with current improvement we add generation of associated records, which is pretty general case (to have associations). Attributes related changes - (13) Fix `[array,enum]` issue. Potentially nested arrays like `[array,[array,enum]]` should work as well (for now it is outside of this change scope). - (14) Add `array` alias for `[array,string]`. - (15) Fix possible collision of `enum` values with options. Extract enum values into specific options. Add support both variants: string option `[one,two]`, integer option `[[one,1],[two,2]]`. Adjust migration and other files representation. - (16) Add option-flag `required` to use in migration, schema and input form in html and live. When no attribute marked as required, the first given attribute is set to be required, with provided notification and confirmation message in console. - (17) Add alias `*` for option-flag `required`. - (18) Add option-flag `virtual` to use in schema and field skipping in migration. Add support of type `any` to be used only with this flag (add validation logic). - (19) Add option-flag `index` to use in migration. - (20) Add `default,value` option for types `boolean`, `integer`, `decimal`, `float`. - (21) Add `precision,value` and `scale,value` options for `decimal` type, to use in migration and test files. Add validations: `scale` can be used only with `precision`, `scale` have to be less then `precision`, minimum for `scale` is `1`, minimum for `precision` is `2`. - (22) Add option `Context.Schema` for `references` type. This options should be used when referenced schema cannot be inferred from the attribute name (e.g. when it has different context). Referenced table and type will be properly inferred from schema reflection. It should fix issue with incorrect references type in case when schema created `--binary-id` option, but referenced schema was created with general `id`. And vice versa. - (23) Add option `assoc,value` for `references` type. For cases when it cannot be inferred from the attribute name. - (24) Add option `on_delete,value` for `references` type. Pretty often we want to deviate from default value `nothing`. - (25) Add option `column,value` for `references` type. For cases when referenced column differs from default `id`. - (26) Add option `type,value` for `references` type. For rear cases when referenced schema is not reachable (e.g. in generators' tests), or when referenced column is not default `id`. For now support values `id`, `binary_id`, `string`. - (27) Add option `table,value` for `references` type. For rear cases when referenced schema is not reachable (e.g. in generators' tests). - (28) Update guides about changes in `references` interface and other parts. Notes NOTE: (29) One drawback so far is necessity to specify `string` type if any option is provided. Which can be neglected I think. Otherwise we need more specific separation between name-type and options, something in a way of `name:type|options`. Then it would be easy and safe to parse omitted string type in `name|options`. Let me know if you want apply this approach. it should be easy enough to modify. NOTE: (30) There is bug in current original version: live test fail in case of invalid attributes, when only boolean attributes exists (cannot find `"can&phoenixframework#39;t be blank"` text.). An this bug still exists in this PR. It is out of scope of this PR to decide approach to fix it.
@SteffenDE Just noticed unfortunate for me last minute commit into main branch by you. I guess it is karma for relying on long time not updated logic without previous announcement of ideas to change 😄 I will close this PR, resolve conflict and create new PR with updated changes. Meanwhile, maybe you will be interested to observe changes I did, before deciding merging anything else related to generators? 🙇🏼 |
Fixes #5987
Points below have numbers for references in PR discussions.
Structural and general changes
Mix.Phoenix.Attribute
module. It parses cli attribute intoAttribute
struct, with validation of types and options, and prefilling some default options. Covered with tests.Mix.Phoenix.Migration
module. It prepares data, based onAttribute
struct, to apply in migration file. Covered with tests. This eliminate migration data preparation in cases with no migration generation (e.g. for embedded schema or when flag is passed). Thoughts for later: potentially this logic can be used in newmix phx.gen.migration
, as extension ofmix echo.gen.migration
with attributes.Mix.Phoenix.TestData
module. It prepares data, based onAttribute
struct, to apply in test files. Covered with tests.Mix.Phoenix.Schema
, based onAttribute
struct. Only parsing cli attributes and preparing generalsample_values
is done during creation ofSchema
struct. Specific data for generated files are created on demand based onschema.attrs
andschema.sample_values
.Mix.Phoenix.Web
module. It prepares data, based onAttribute
struct, to apply in html, live files. Covered with tests. It also postpone data generation. Thus e.g. data generation will be invoked only if conflicts check passes.Mix.Phoenix.indent_text
to indent text or list of lines with spaces, and prepend and append empty lines when needed. Use it for new and old refactored cases.context
files beforejson
,html
,live
files, as it doesn't need specific extra bindings."#{[1,2]}"
is<<1, 2>>
,"#{[42,43]}"
is"*+"
). And it even leads to errors during run of generated tests (e.g."#{[1042,1043]}"
leads to error(ArgumentError) lists in Phoenix.HTML templates only support iodata, and not chardata.
). Correct with simple default rendering logic - adjustment with(values || []) |> List.flatten() |> Enum.join(", ")
. It works for any levels nesting arrays. And feels reasonable default representation for array values.create_attrs
andupdate_attrs
from module attributes intotest
body. More practical approach, as more often we need to generate some data. E.g. with current improvement we add generation of associated records, which is pretty general case (to have associations).Attributes related changes
[array,enum]
issue. Potentially nested arrays like[array,[array,enum]]
should work as well (for now it is outside of this change scope).array
alias for[array,string]
.enum
values with options. Extract enum values into specific options. Add support both variants: string option[one,two]
, integer option[[one,1],[two,2]]
. Adjust migration and other files representation.required
to use in migration, schema and input form in html and live. When no attribute marked as required, the first given attribute is set to be required, with provided notification and confirmation message in console.*
for option-flagrequired
.virtual
to use in schema and field skipping in migration. Add support of typeany
to be used only with this flag (add validation logic).index
to use in migration.default,value
option for typesboolean
,integer
,decimal
,float
.precision,value
andscale,value
options fordecimal
type, to use in migration and test files. Add validations:scale
can be used only withprecision
,scale
have to be less thenprecision
, minimum forscale
is1
, minimum forprecision
is2
.Context.Schema
forreferences
type. This options should be used when referenced schema cannot be inferred from the attribute name (e.g. when it has different context). Referenced table and type will be properly inferred from schema reflection. It should fix issue with incorrect references type in case when schema created--binary-id
option, but referenced schema was created with generalid
. And vice versa.assoc,value
forreferences
type. For cases when it cannot be inferred from the attribute name.on_delete,value
forreferences
type. Pretty often we want to deviate from default valuenothing
.column,value
forreferences
type. For cases when referenced column differs from defaultid
.type,value
forreferences
type. For rear cases when referenced schema is not reachable (e.g. in generators' tests), or when referenced column is not defaultid
. For now support valuesid
,binary_id
,string
.table,value
forreferences
type. For rear cases when referenced schema is not reachable (e.g. in generators' tests).references
interface and other parts.Notes
NOTE: (29) One drawback so far is necessity to specify
string
type if any option is provided. Which can be neglected I think. Otherwise we need more specific separation between name-type and options, something in a way ofname:type|options
. Then it would be easy and safe to parse omitted string type inname|options
. Let me know if you want apply this approach. it should be easy enough to modify.NOTE: (30) There is bug in current original version: live test fail in case of invalid attributes, when only boolean attributes exists (cannot find
"can't be blank"
text.). An this bug still exists in this PR. It is out of scope of this PR to decide approach to fix it.