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.

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

Generate struct (with RubyFileGenerator, add RubyFileWriter) #199

Merged
merged 28 commits into from
Jul 14, 2024

Conversation

cllns
Copy link
Member

@cllns cllns commented Jul 13, 2024

This adds a 'struct' generator, and some abstractions, a Hanami::CLI::Commands::App::Generate::Command, which handles the creation of a generator object, as well as calling the generator.

We also add a RubyFileWriter class (I don't love the name but it seems OK). This determines what gets passed to RubyFileGenerator, deciding on name of the class, the body, the modules it's nested in, the parent, etc. It's got 8 initializer kwarg params which is a bit much. It can be refactored later but for now it's an implementation detail so I think it's OK. We could break out some of them (relative_parent_class, extranamespace, body) into args on call but then we have to pass them around more, too. Happy to do that if we prefer, though.

Note that this PR is against the branch for #186.

Copy link
Member

@timriley timriley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great, let's get this in!

A couple of tidy-up notes, feel free to take care of those before merging, no further review necessary 👍🏼

Comment on lines +34 to +37
def generator_class
# Must be implemented by subclasses, with class that takes:
# fs:, inflector:, out:
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh, you did an equivalent to what I suggested in #186, so we're deinfitely on the same page here 😄

Comment on lines 3 to 7
require "dry/inflector"
require "dry/files"
require "shellwords"
require_relative "../../../naming"
require_relative "../../../errors"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
require "dry/inflector"
require "dry/files"
require "shellwords"
require_relative "../../../naming"
require_relative "../../../errors"
require "dry/inflector"
require "dry/files"
require "shellwords"
require_relative "../../../naming"
require_relative "../../../errors"

I don't think we need any of these requires here anymore! Feel free to take them out of this as well as the Generate::Operation command before merging 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, great! Gone

Comment on lines +18 to +25
fs:,
inflector:,
app_namespace:,
key:,
slice:,
relative_parent_class:,
extra_namespace: nil,
body: []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like you mentioned in the description, this is an internal class only so we can tolerate some slightly less ergonomic API design, but I think a refactor to allow a FileWriter instance to be initialised once and then used to write multiple files might be nice.

So this would probably pull the fs: and inflector: params out into #initialize, with the rest going into some method that writes an actual file.

I think this'd be worth playing with sometime, but it's not blocking and I'm definitely keen to get our full range of CLI commands ready for beta1 on Tuesday. So happy if this comes in a follow-up change of some kind :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a note for myself to explore this later

Comment on lines 12 to 14
def output
out.rewind && out.read.chomp
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def output
out.rewind && out.read.chomp
end
def output
out.string.chomp
end

I realised there's a way nicer way to do this that avoids mutating the IO object itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet. Did this for struct and operation, and made a note to self to fix these for the other generators when I switch them over to using this RubyFileGenerator/Writer

end

context "generating for a slice" do
it "generates a struct in a top-level namespace, with recommendation" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it "generates a struct in a top-level namespace, with recommendation" do
it "generates a struct in a top-level namespace" do

I think "with recommendation" might just be a hangover from the operation generator tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Busted! Fixed this

module App
# @since 2.2.0
# @api private
class Struct
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new generators are so tidy 😍

cllns and others added 2 commits July 13, 2024 19:42
…ad of ERB (#186)

* Add dry-operation to default Gemfile

* Add base Operation class, based on dry-operation

* Fix view spec

* Add Operation generators

* Add empty `call` method definition

* Remove ostruct

* Allow slash separator for generator

* Allow slash separator for generator

* Rename module to admin

* Remove newlines in generated files

By adding new templates for un-nested operations

* Remove input as default args

* Remove Operations namespace, generate in app/ or slices/SLICE_NAME/

* Prevent generating operation without namespace

* Revert "Prevent generating operation without namespace"

This reverts commit a5bd2f3.

* Add recommendation to add namespace to operations

* Change examples

* Switch to outputting directly, remove Files#recommend

* Add Hanami::CLI::RubyFileGenerator

* x.x.x => 2.2.0

* x.x.x => 2.2.0

* Include Dry::Monads[:result] in base Action

* Add .module tests

* Convert top-level app operation to use RubyFileGenerator

* Convert nested app operation to use RubyFileGenerator

* Support slash separators

* Convert top-level slice operation to use RubyFileGenerator

* Remove OperationContext

* Remove namespaces instance variable

* Refactor to variables

* Remove last temporary instance variable

* Refactor

* More refactoring, for clarity

* Rename variable for clarity

* Rename helper method

* Simplify RubyFileGenerator, support older versions

* Convert Operation generator to use simplified RubyFileGenerator

* Remove un-used errors

* Refactor

* Older kwargs forwarding style

* Refactor

* Rename variable

* Add explanatory comment

Add dry-monads include for slice base action

* Fix base slice action

* Remove un-used ERB templates

* Remove OperationContext

* Ternary over and/or

* Fix missing 'end' from bad merge

* Fix namespace recommendation

* Extract App::Generate::Command

* Specify full name, to use App::Command

* Use constants file

* Move class methods above initialize

* Use constants file

* Add yard comments

* Revert "Use constants file"

This reverts commit 303f502.

Would need to namespace it and we may want to this to standalone so
keeping it here. It's just two little spaces anyway

* Fix indent to be two spaces
@cllns cllns force-pushed the generate-struct branch from 8ba77d3 to 5fac400 Compare July 14, 2024 01:55
@cllns cllns merged commit cb06023 into add-file-generator Jul 14, 2024
3 checks passed
This was referenced Jul 14, 2024
cllns added a commit that referenced this pull request Jul 14, 2024
* Add dry-operation to default Gemfile

* Add base Operation class, based on dry-operation

* Fix view spec

* Add Operation generators

* Add empty `call` method definition

* Remove ostruct

* Allow slash separator for generator

* Allow slash separator for generator

* Rename module to admin

* Remove newlines in generated files

By adding new templates for un-nested operations

* Remove input as default args

* Remove Operations namespace, generate in app/ or slices/SLICE_NAME/

* Prevent generating operation without namespace

* Revert "Prevent generating operation without namespace"

This reverts commit a5bd2f3.

* Add recommendation to add namespace to operations

* Change examples

* Switch to outputting directly, remove Files#recommend

* Add Hanami::CLI::RubyFileGenerator

* x.x.x => 2.2.0

* x.x.x => 2.2.0

* Include Dry::Monads[:result] in base Action

* Add .module tests

* Convert top-level app operation to use RubyFileGenerator

* Convert nested app operation to use RubyFileGenerator

* Support slash separators

* Convert top-level slice operation to use RubyFileGenerator

* Remove OperationContext

* Remove namespaces instance variable

* Refactor to variables

* Remove last temporary instance variable

* Refactor

* More refactoring, for clarity

* Rename variable for clarity

* Rename helper method

* Simplify RubyFileGenerator, support older versions

* Convert Operation generator to use simplified RubyFileGenerator

* Remove un-used errors

* Refactor

* Older kwargs forwarding style

* Refactor

* Rename variable

* Add explanatory comment

Add dry-monads include for slice base action

* Fix base slice action

* Remove un-used ERB templates

* Remove OperationContext

* Ternary over and/or

* Fix missing 'end' from bad merge

* Fix namespace recommendation

* Extract App::Generate::Command

* Specify full name, to use App::Command

* Use constants file

* Move class methods above initialize

* Use constants file

* Add yard comments

* Revert "Use constants file"

This reverts commit 303f502.

Would need to namespace it and we may want to this to standalone so
keeping it here. It's just two little spaces anyway

* Fix indent to be two spaces

* Generate struct (with RubyFileGenerator, add RubyFileWriter) (#199)

* Add struct generator

* Extract App::Generate::Command

* Specify full name, to use App::Command

* Use App::Generate::Command for Struct

* Use KEY_SEPARATOR from constants file

* Provide generator class to command

* Extract Helper, use for Struct generator

* Fix specs with Helper

* kwargs

* Rename helper to RubyFileWriter

* Fix examples

* Remove optional kwarg

* Reorder args

* Rename to relative_parent_class

* Remove duplicate implementation

* Add api doc comments

* Reorder methods

* Refactor initialize

* Refactor to use method instead of arg

* Refactor to move logic into generator

* Reorder assignments

* Add Hanami::CLI::RubyFileGenerator, convert Operation to use it instead of ERB (#186)

* Add dry-operation to default Gemfile

* Add base Operation class, based on dry-operation

* Fix view spec

* Add Operation generators

* Add empty `call` method definition

* Remove ostruct

* Allow slash separator for generator

* Allow slash separator for generator

* Rename module to admin

* Remove newlines in generated files

By adding new templates for un-nested operations

* Remove input as default args

* Remove Operations namespace, generate in app/ or slices/SLICE_NAME/

* Prevent generating operation without namespace

* Revert "Prevent generating operation without namespace"

This reverts commit a5bd2f3.

* Add recommendation to add namespace to operations

* Change examples

* Switch to outputting directly, remove Files#recommend

* Add Hanami::CLI::RubyFileGenerator

* x.x.x => 2.2.0

* x.x.x => 2.2.0

* Include Dry::Monads[:result] in base Action

* Add .module tests

* Convert top-level app operation to use RubyFileGenerator

* Convert nested app operation to use RubyFileGenerator

* Support slash separators

* Convert top-level slice operation to use RubyFileGenerator

* Remove OperationContext

* Remove namespaces instance variable

* Refactor to variables

* Remove last temporary instance variable

* Refactor

* More refactoring, for clarity

* Rename variable for clarity

* Rename helper method

* Simplify RubyFileGenerator, support older versions

* Convert Operation generator to use simplified RubyFileGenerator

* Remove un-used errors

* Refactor

* Older kwargs forwarding style

* Refactor

* Rename variable

* Add explanatory comment

Add dry-monads include for slice base action

* Fix base slice action

* Remove un-used ERB templates

* Remove OperationContext

* Ternary over and/or

* Fix missing 'end' from bad merge

* Fix namespace recommendation

* Extract App::Generate::Command

* Specify full name, to use App::Command

* Use constants file

* Move class methods above initialize

* Use constants file

* Add yard comments

* Revert "Use constants file"

This reverts commit 303f502.

Would need to namespace it and we may want to this to standalone so
keeping it here. It's just two little spaces anyway

* Fix indent to be two spaces

* Remove extraneous requires

* Use out.string.chomp

* Fix name of expectation
@timriley timriley deleted the generate-struct branch July 14, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants