GUILD-690: Initial draft of new Active Record Consumer Generator by jenchanflipp · Pull Request #170 · flipp-oss/deimos

jenchanflipp · 2022-11-15T21:40:35Z

Description

To create an ActiveRecordConsumer (one of our most common tasks) there are a number of steps that have to be taken - create a DB migration, Rails model, consumer, consumer config, Deimos schema class. We have now added a new ActiveRecordConsumer Generator, which will streamline this process into a single flow.

Notes for consideration:

Currently the generator runs with command line options. The only mandatory argument is full schema name. There is one optional second argument, where the user can pass in an existing config file to add the Deimos config to.
Question: Are all the above files absolutely necessary when creating a new Active Record Consumer so it is ok to automatically generate them all? Or should add some additional arguments where a user could specify, say, that they would not want to generate a Deimos schema class.
Question: Is it preferred to have an interactive flow where the user is prompted with questions and can provide user input instead of having to specify arguments on the command line?

Fixes # (GUILD-690)

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Test A: Unit tests via rspec where we are invoking the generator and validating that all the necessary files have been created
See spec/generators/active_record_consumer_generator_spec.rb
Test B: Manual tests that invoke the generator via command line in a ruby project that imports the deimos gem
Ensure you have a Widget schema file located in your schema root and then execute command rails g deimos:active_record_consumer test.Widget and rails g deimos:active_record_consumer test.Widget config/initializers/myconfig.rb

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added a line in the CHANGELOG describing this change, under the UNRELEASED heading
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

dorner · 2022-11-16T15:32:03Z

@@ -0,0 +1,30 @@
+class <%= migration_class_name %> < ActiveRecord::Migration<%= migration_version %>


Don't we already have this template file? We should be able to reuse it.

Yes - better idea to reuse the existing code - will fix this.

dorner · 2022-11-16T15:33:27Z

+          config_file_path = "#{initializer_path}/#{config_file}"
+          config_file_path = config_path if config_path.present?
+
+          if File.exist?(config_file_path)


We should validate that the config file exists if it's passed in. If not, we can create it.

When I tested it by passing in a config file path that did not already exist, it did go ahead and create that config file.

Sorry, that was unclear. We should only create it if no path was specified. If a path was specified and doesn't exist, we should assume that means that they made a mistake and raise an error.

Actually, I'm not sure we need to create it at all. If we want to auto-create a config file, it probably would be in an installer or something, but I don't think we have a use case for that. So maybe I'd just say don't create anything, and throw an error if the file doesn't exist.

dorner · 2022-11-16T15:34:26Z

+
+          if File.exist?(config_file_path)
+            config_template = File.expand_path(find_in_source_paths('config.rb'))
+            insert_into_file(config_file_path.to_s, CapturableERB.new(::File.binread(config_template)).result(binding))


Wouldn't this insert the whole file? We need a way to just insert the consumer do block.

Ok, I will change it to just insert the consumer do block. Wasn't sure if we needed to add the other config.

dorner · 2022-11-16T15:35:16Z

+        # @return [String]
+        def consumer_name
+          "#{schema.classify}Consumer"
+        end


A lot of this is copied from the migration template. There should be a way to just call the migration generator from this generator rather than having the same code twice.

I considered extending from the migration template but decided it wasn't the right approach. Will find a better way!

dorner · 2022-11-16T15:36:46Z

+
+        # @return [String] Returns the name of the first field in the schema, as the key
+        def key_field
+          fields.first.name


This is almost definitely not going to be the right approach. It probably makes more sense to just have the key config specified as part of the input. We'll also need the key schema (if there is one) when generating the schema classes.

Ok, yes makes sense to include key config as an input.

In terms of the key schema, how does that work? I noticed that the schema glasses generator doesn't take in any arguments. So, if the user has a key schema for the new consumer, do they just have to place it in the SCHEMA_ROOT prior to running the ActiveRecordConsumer generator? And then what will be the difference in output? Will the generated schema classes be different?

The generator reads the configuration which already knows what the key schema is.

The generator has a method generate_classes(schema_name, namespace, key_config) which you should probably be using.

dorner · 2022-11-16T15:37:29Z

+
+        # Generates schema classes
+        def create_deimos_schema_class
+          Deimos::Generators::SchemaClassGenerator.start


There should be a way to generate just the one schema class we care about.

Which is the one schema class that we care about?

The one that was passed in as an input.

dorner · 2022-11-16T15:38:37Z

This is a great first pass! I do think that having an interactive process will make more sense, especially because right now the templates are making a bunch of assumptions as to what needs to be filled in. Allowing the user to specify that will help a lot so they don't have to go into each of these files and edit them.

jenchanflipp · 2022-11-16T15:59:52Z

This is a great first pass! I do think that having an interactive process will make more sense, especially because right now the templates are making a bunch of assumptions as to what needs to be filled in. Allowing the user to specify that will help a lot so they don't have to go into each of these files and edit them.

Thanks for your feedback! An interactive process does sound like it would be more helpful to the user. So, the user would have to specify

existing config file name
key config

Is there anything else that they should provide?

dorner · 2022-11-16T17:03:19Z

I'd look through the template and generator files to see what assumptions are being made in the output, and try to add those as inputs.

…with new option to skip generating classes for all schema files

… deimos consumer config

jenchanflipp added 2 commits November 15, 2022 16:38

GUILD-690: Initial draft of new Active Record Consumer Generator

65c8d5d

GUILD-690: Initial commit of ActiveRecordConsumerGenerator code

e07ae14

dorner requested changes Nov 16, 2022

View reviewed changes

jenchanflipp added 5 commits January 24, 2023 15:08

GUILD-690: Invoke active record generator and schema class generator …

8b3130d

…with new option to skip generating classes for all schema files

GUILD-690: Validate if passed in config file exists and only generate…

3e41478

… deimos consumer config

GUILD-690: Handling config file edge cases

1222c74

GUILD-690: Add support for specifying key_config arg

1ee2615

GUILD-690: Deleting unused snapshots

ce5b340

		@@ -0,0 +1,30 @@
		class <%= migration_class_name %> < ActiveRecord::Migration<%= migration_version %>

Conversation

jenchanflipp commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorner commented Nov 16, 2022

Uh oh!

jenchanflipp commented Nov 16, 2022

Uh oh!

dorner commented Nov 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jenchanflipp commented Nov 15, 2022 •

edited

Loading