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.

Sign up for GitHub

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

Jump to bottom

Improve generators attributes #5987

Open
ShPakvel opened this issue Nov 26, 2024 · 0 comments
Open

Improve generators attributes #5987

ShPakvel opened this issue Nov 26, 2024 · 0 comments

Comments

@ShPakvel
Copy link
Contributor

Topic

CLI attributes format for mix phx.gen.* tasks.

Current format all:values:separated:with:only:colons

It is very simple but not flexible and robust.
Which make underneath code more coupled and fragile with every enhancement.
Hence leading to clashes, like in examples bellow.

Issue(s)

Core issue is that different parts of an attribute info are mixed and coupled in very simple format.
There is no clear boundaries to extract specifics of types and general options.

Examples:

This issue describes already existing bug with compound :array:enum:value type. Which should be allowed according to the task description.

Another issue with enum values clash to options.
Let's say we need value redact or unique. Like stages:enum:publish:review:redact.
Right now intended enum value redact will be treated as option redact: true for schema field stages.

Similarly, if we want to enhance options of attribute, they can clash with other parts.
E.g. my initial idea for small feature.
I wanted to add field option required (maybe with alias *). And use it to generate proper representation across layers:

  • In migration, add null: false constraint to related column.
  • In schema, add only such marked fields into list of validate_required([...]) logic.
  • In html form, add required attribute to related input.
    This easily can clash for instance with enum value as previous already existing clashes described.

Another hiccup I met when tried to follow phoenix guide.
When decimal column in migration was manually provided with options precision: 15, scale: 6 before migrating generated code.
It led to failing tests with previously generated default values in fixtures in wrong values/format.
Which could be nicely resolved with options for precision and scale.

With references at the beginning I was surprised how little we generate for general behavior, and need to update manually after.
Digging a bit of code history I encountered issue related to possible context differences.

  • One way to resolve it could be replacing field_name:references:table with field_name:references:Context.Schema. We generate in scope of phx.gen.schema after all. Having specified referenced schema we could infer necessary info from its reflection. For instance, needed first of all __schema__(:source), as well as correct referenced column type.

Solution Proposal

Main goal.
Keep attribute CLI format simple for general cases, while improving flexibility for type and options enhancements for specific cases.

So main change we want to do is for clear separation of types from options, and among options.

My suggestion is

Attribute cli format in details.
There are still 3 main parts of attribute format: name, type, options. They divided by ":".

  • name is always present.
  • type is one of the valid values. Default to string. Array presented as [array,inner_type].
  • options is a list separated by ":". An option mimics its counterpart from mentioned functions. Some options can be mandatory, see enum case for example. Some options are for enhanced code generation. Option can use characters , [, ] to format details.

With this we can improve format parsing.
Make it more independent per type and per option.
Thus simpler logic with less clashes and hence bugs.

I will attach PR with solution soon.
@ShPakvel ShPakvel mentioned this issue Nov 26, 2024
Closed
ShPakvel added a commit to ShPakvel/phoenix that referenced this issue Nov 26, 2024
@ShPakvel
Improve generators attributes
816ab04 
Fixes phoenixframework#5987

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.
ShPakvel added a commit to ShPakvel/phoenix that referenced this issue Nov 26, 2024
@ShPakvel
Improve generators attributes
12b5a23 
Fixes phoenixframework#5987

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.
ShPakvel added a commit to ShPakvel/phoenix that referenced this issue Nov 27, 2024
@ShPakvel
Improve generators attributes
a2150f7 
Fixes phoenixframework#5987

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) Rely on correct type inferred from referenced schema for associations, get rid of unneeded any more general foreign type setup as schema module attribute. It original implementation there it was a bug when referenced schema had different type.
- (7) 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.
- (8) 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.
- (9) Generate `context` files before `json`, `html`, `live` files, as it doesn't need specific extra bindings.
- (10) Extract fixture data generation, to be done only when fixture is going to be created.
- (11) Extract html assertion data generation, to be done only when web files is going to be created.
- (12) 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.
- (13) 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

- (14) Fix `[array,enum]` issue. Potentially nested arrays like `[array,[array,enum]]` should work as well (for now it is outside of this change scope).
- (15) Add `array` alias for `[array,string]`.
- (16) 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.
- (17) 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.
- (18) Add alias `*` for option-flag `required`.
- (19) 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).
- (20) Add option-flag `index` to use in migration.
- (21) Add `default,value` option for types `boolean`, `integer`, `decimal`, `float`.
- (22) 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`.
- (23) 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.
- (24) Add option `assoc,value` for `references` type. For cases when it cannot be inferred from the attribute name.
- (25) Add option `on_delete,value` for `references` type. Pretty often we want to deviate from default value `nothing`.
- (26) Add option `column,value` for `references` type. For cases when referenced column differs from default `id`.
- (27) 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`.
- (28) Add option `table,value` for `references` type. For rear cases when referenced schema is not reachable (e.g. in generators' tests).
- (29) Update guides about changes in `references` interface and other parts.

Notes

NOTE: (1) 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: (2) 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve generators attributes ShPakvel/phoenix
1 participant
@ShPakvel