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

Add dry-operation generators #171

Merged
merged 27 commits into from
Jul 5, 2024
Merged

Add dry-operation generators #171

merged 27 commits into from
Jul 5, 2024

Conversation

cllns
Copy link
Member

@cllns cllns commented Jun 15, 2024

Addresses #166. This creates a base operation in the app, then one in each slice when they're generated. Also adds a hanami generate operation command

Two issues that still need to be addressed:

  1. Generating a top-level operation (i.e. add_book not admin.books.add) has empty lines around the class due to trying to add modules for namespaces that don't exist. The newlines from the ERB template are still there. A simple way to handle it would be nested_app_operation.erb and nested_slice_operation.erb but maybe we can think of something better? I added different templates for top-level and nested operations to solve this. Open to other solutions, but this is simple.

  2. I have an xit for doing admin/books/add because that should be the same as admin.books.add but it's rendering it as class Admin::Books::Add instead of nested modules. Feel free to remove it but I feel like we should support this syntax the same way as the period separated names. Fixed this. This allows hanami generate operation admin/books/add to work just like hanami generate admin.books.add. This will match the folder structure. The only thing here is that now we should add the same functionality to all our generators, which I'm happy to do in a separate PR after this one.

Also, @timriley do we still do x.x.x as the version number to be changed at release time?

Happy to address any feedback, and also feel free to take this WIP and finish it as you wish @timriley, I don't want to hold you up.

@cllns cllns requested a review from timriley June 15, 2024 00:57
@cllns cllns changed the title Add dry operation generators Add dry-operation generators Jun 15, 2024
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.

This is looking great, thanks for putting it together, @cllns!

Biggest piece of feedback from me is that I want us to allow users to generate operations into any place in their apps or slices.

In this way generate operation is just a more specialised version of the generate component command we've recently added.

Have left some inline comments explaining more of my thinking here.

Would you be up for making that adjustment? Thank you! ❤️

lib/hanami/cli/commands/app/generate/operation.rb Outdated Show resolved Hide resolved
raise MissingSliceError.new(slice) unless fs.directory?(slice_directory)

if context.namespaces.any?
fs.mkdir(directory = fs.join(slice_directory, "operations", context.namespaces))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should hardcode "operations" in the directory path here. Operations should be able to be created anywhere within an app or slice.

(This comment also applies to all the other places we've hard-coded "operations" within this PR)

@cllns
Copy link
Member Author

cllns commented Jun 19, 2024

All great feedback, I agree with all of it. Will work to implement it. Still unsure about the exact pattern we want but I can get us closer to allowing operations anywhere, even if we tweak the UX later

@cllns
Copy link
Member Author

cllns commented Jun 19, 2024

Ok I implemented removing the "Operations" folder and namespace, so now it's completely generic.

I also made it (in my latest commit) so that users must provide a namespace. I think this is fair, since app/ (and slices/SLICE_NAME) only have folders and base classes in it. I feel like it's not a great spot for non-abstract classes. People can still move them there after generation if they want, but I don't think we should encourage it. WDYT?

Note that this is different from how component is currently generated, which lets people generate without the namespace. If we do this here, perhaps we should do it there too.

I worry the error message I wrote is too verbose, WDYT?

@timriley
Copy link
Member

I see your point about guiding users away from creating operations in the top-level.

What about issuing a warning instead of a hard-failure? So we'd still allow it to be created, but also give a hint to the user about possibly better approaches. And we could allow those warnings to be turned off with a --no-recommendations flag or something.

My thinking here is that Hanami is not really providing hard-and-fast guidelines about user domain code at this point, so I think it's better to err on the side of flexibility.

Plus, I think if I wanted a top-level operation, I'd be pretty annoyed if our CLI didn't let me generate one.

If that all sounds like too much work, then I'm also fine with just allowing top-level operations without any kinds of warnings or errors or anything of the like.

@cllns
Copy link
Member Author

cllns commented Jun 20, 2024

Ok, I got rid of the Error and now we have it as puts-ing a "Recommendation:" that's indented two spaces to visually distinguish it from the other output.

I added Hanami::CLI::Files#recommend, which is a slightly awkward place since it's just outputting, but it's related to file generation so I think it's a practical place for it.

I started to add skip_recommendations as CLI arg but it's awkward and ends up touching a lot of stuff, so maybe we can wait until people complain to build that out?

@cllns
Copy link
Member Author

cllns commented Jun 20, 2024

I just integrated dry-operation into my app and I had to add include Dry::Monads[:result] in a couple spots. In my app it was app/action.rb and spec/spec_helper.rb (where I also had to require "dry/monads" and config.include) in order to be able to match on the result.

I was thinking we could just include it generally within the main App namespace by default, but the specs use inline syntax so that doesn't work due to nesting.

Where should we include Dry::Monads[:result] when an operation is generated? At the very least we need to output a message instructing the users to include it, but ideally we could just include it in a few key spots when generating the first operation so it would Just Work. We could also add it when generating a new app, but the M-word scares people sometimes.

@cllns
Copy link
Member Author

cllns commented Jun 20, 2024

Also, should we add ROM transaction support to the default Operation if hanami-db is loaded? Related to #180

I think it would be something like:

class MyApp
  class Operation < Dry::Operation
    include Dry::Operation::Extensions::ROM
    include Deps[rom: "db.rom"]
  end
end

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.

Nearly there! Just a couple last thoughts. Thanks for bearing with me here, @cllns :)

lib/hanami/cli/files.rb Outdated Show resolved Hide resolved
else
fs.mkdir(directory = fs.join(slice_directory))
fs.write(fs.join(directory, "#{context.name}.rb"), t("top_level_slice_operation.erb", context))
fs.recommend("Add a namespace to operation names, so they go into a folder within #{directory}/.")
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about this for wording?

Suggested change
fs.recommend("Add a namespace to operation names, so they go into a folder within #{directory}/.")
fs.recommend(%(Generating a top-level operation. To generate into a directory, add a namespace: "my_namespace.#{context.name}))

That first sentence is there to explain why we're offering the recommendation in the first place. The second sentence is there to give a bit more guidance as to what the "recommended" args might look like.

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, switched it to this. I added the indentation too so it's more noticeable.

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.

@cllns Looks good! Let's add @since 2.2.0 instead of @since x.x.x. After that's done, feel free to merge! Thank you!

@timriley timriley assigned timriley and cllns and unassigned timriley Jun 22, 2024
@cllns
Copy link
Member Author

cllns commented Jun 22, 2024

I just integrated dry-operation into my app and I had to add include Dry::Monads[:result] in a couple spots. In my app it was app/action.rb and spec/spec_helper.rb (where I also had to require "dry/monads" and config.include) in order to be able to match on the result.

I was thinking we could just include it generally within the main App namespace by default, but the specs use inline syntax so that doesn't work due to nesting.

Where should we include Dry::Monads[:result] when an operation is generated? At the very least we need to output a message instructing the users to include it, but ideally we could just include it in a few key spots when generating the first operation so it would Just Work. We could also add it when generating a new app, but the M-word scares people sometimes.

I also added include Dry::Monads[:result] into the base app Action, so people can match on the result of operations in their actions with just Success(value) instead of needing to write Dry::Monads::Result::Success in every spot they use a dry-operation. What do we think about this? If we do this, I can also make a PR to hanami-rspec to add the require and config.include Dry::Monads[:result] in the rspec support file, so testing operations just works in the same way.

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.

Thanks again @cllns! This is looking good :)

I also added include Dry::Monads[:result] into the base app Action, so people can match on the result of operations in their actions with just Success(value) instead of needing to write Dry::Monads::Result::Success in every spot they use a dry-operation. What do we think about this?

Yep, I think this is great!

I left two tiny pieces of feedback about this via inline comment. Once those are done, let's merge this!

If we do this, I can also make a PR to hanami-rspec to add the require and config.include Dry::Monads[:result] in the rspec support file, so testing operations just works in the same way.

Yeah, that would be awesome!

@@ -5,5 +5,6 @@ require "hanami/action"

module <%= camelized_app_name %>
class Action < Hanami::Action
include Dry::Monads[:result]
Copy link
Member

@timriley timriley Jun 29, 2024

Choose a reason for hiding this comment

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

Suggested change
include Dry::Monads[:result]
# Provide `Success` and `Failure` for pattern matching on operation results
include Dry::Monads[:result]

I like that you've included this!

I suggest we include a little explainer to make it clear why this line is here.

Copy link
Member

@timriley timriley Jun 29, 2024

Choose a reason for hiding this comment

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

For consistency, can you please update the slice generator to include this in slice actions too?

The difference should be that the slice generator should check to see if dry-monads is bundled before it includes this line. If the user chooses to remove dry-operation from their Gemfile, then dry-monads will disappear and the include would then result in an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this for the slice, but the slice actions inherit from app/action.rb so it doesn't do anything. I kept it in since I might be missing something, but I think we can remove it?

I also had to add a line to require dry/monads

Copy link
Member

Choose a reason for hiding this comment

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

@cllns Ah yeah, sorry, that's my mistake. With slice actions inheriting from an app action, we don't need to put anything additional into the slice actions.

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.

This is still looking great, thanks @cllns!

I just noted here that we indeed don't need to be touching slice actions — sorry for the runaround there.

Once that's sorted out again, this looks good to merge!

@cllns cllns merged commit 2ffa120 into main Jul 5, 2024
6 checks passed
@cllns cllns deleted the add-dry-operation branch July 5, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants