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 service generator #11265

Merged
merged 8 commits into from
Nov 10, 2020
Merged

Add service generator #11265

merged 8 commits into from
Nov 10, 2020

Conversation

citizen428
Copy link
Contributor

@citizen428 citizen428 commented Nov 4, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

I'm currently a bit blocked on profile work, waiting for 2 PRs to get approved and merged. So in the meantime I did some work on a service generator to standardize our service objects a bit more.

Examples:

Generate a non-namespaced service without arguments

$ rails generate service DoTheThing

# app/services/do_the_thing.rb
class DoTheThing
  def self.call
    new.call
  end

  def call
  end
end
# spec/services/do_the_thing_spec.rb
require "rails_helper"

RSpec.describe DoTheThing, type: :service do
  pending "add some examples to (or delete) #{__FILE__}"
end

Generate a non-namespaced service with arguments:

$ rails generate service DoTheThing arg1 arg2

# app/services/do_the_thing.rb
class DoTheThing
  def self.call(arg1, arg2)
    new(arg1, arg2).call
  end

  def initialize(arg1, arg2)
    @arg1 = arg1
    @arg2 = arg2
  end

  def call
  end
end

The generated spec is the same as above.

Generate a namespaced service with arguments

$ rails generate service things/dothem arg1 arg2

# app/services/things/do_them.rb
class Things::DoThem
  def self.call(arg1, arg2)
    new(arg1, arg2).call
  end

  def initialize(arg1, arg2)
    @arg1 = arg1
    @arg2 = arg2
  end

  def call
  end
end
# spec/services/things/do_them_spec.rb
require "rails_helper"

RSpec.describe Things::DoThem, type: :service do
  pending "add some examples to (or delete) #{__FILE__}"
end

Related Tickets & Documents

n/a

QA Instructions, Screenshots, Recordings

Try the different examples above and verify that the generated code matches your expectations.

Added tests?

  • Yes
  • No, and this is why: there's surprisingly little documentation around testing generators with RSpec. The examples I found all use an external gem for this. I'm not sure I want to add this dependency just yet and we are also not testing the other generator. As this is a convenience tool for developers and not production code I think this is ok until I can evaluate testing strategies during the next cooldown.
  • I need help with writing tests

Added to documentation?

  • Docs.forem.com
  • README
  • No documentation needed

@pr-triage pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Nov 4, 2020
@citizen428 citizen428 requested review from vaidehijoshi, a team, Ridhwana, jacobherrington and benhalpern and removed request for a team November 6, 2020 05:15
@citizen428 citizen428 marked this pull request as ready for review November 6, 2020 05:15
@citizen428 citizen428 requested a review from a team November 6, 2020 05:15
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Nov 6, 2020
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Cool!

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Nov 6, 2020
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Nov 9, 2020
@rhymes
Copy link
Contributor

rhymes commented Nov 9, 2020

@citizen428 the build is failing because of a naming issue:

/home/travis/build/forem/forem/vendor/bundle/ruby/2.7.0/gems/zeitwerk-2.4.0/lib/zeitwerk/loader/callbacks.rb:17:in `on_file_autoloaded': expected file /home/travis/build/forem/forem/lib/generators/service/service_generator.rb to define constant Generators::Service::ServiceGenerator, but didn't (Zeitwerk::NameError)

@citizen428
Copy link
Contributor Author

@rhymes You caught me during lunch, I had already started working on this. The error you posted is a result of an attempt to fix it. Before it failed with this:

/home/travis/build/forem/forem/lib/generators/service/service_generator.rb:1:in `<main>': uninitialized constant Rails::Generators (NameError)

I was a bit dumbfounded, as we do everything the same way as in the data update generator. However, some googling seems to suggest that explicitly requiring rails/generators is the ThingToDo™ since Rails 3:

But this wasn't necessary for the DataUpdateGenerator... Anyway, adding it does lead to the error you mentioned above, which makes sense with how Zeitwerk loads constants.

@citizen428 citizen428 requested a review from a team as a code owner November 9, 2020 05:43
@citizen428
Copy link
Contributor Author

This made me believe we manually updated the autoload path for the other generator and that's indeed the case:

# config/initializers/zeitwerk.rb
# Ignoring folders that don't adhere to the new naming conventions
Rails.autoloaders.main.ignore(Rails.root.join("lib/data_update_scripts"))
Rails.autoloaders.main.ignore(Rails.root.join("lib/generators/data_update"))

I added a line for the new generator now.

@citizen428
Copy link
Contributor Author

Oops, accidentally merge master back here instead of some other branch...

Copy link
Contributor

@Ridhwana Ridhwana left a comment

Choose a reason for hiding this comment

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

Looks good, I just suggested a change for what looks like a typo.

docs/backend/service-objects.md Outdated Show resolved Hide resolved
@pr-triage pr-triage bot removed the PR: unreviewed bot applied label for PR's with no review label Nov 9, 2020
@pr-triage pr-triage bot added the PR: partially-approved bot applied label for PR's where a single reviewer approves changes label Nov 9, 2020
Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

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

🔥

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Nov 10, 2020
@citizen428 citizen428 merged commit 824e9a1 into master Nov 10, 2020
@citizen428 citizen428 deleted the citizen428/add-service-generator branch November 10, 2020 02:09
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: unreviewed bot applied label for PR's with no review labels Nov 10, 2020
narender2031 pushed a commit to SkynoxTech/forem that referenced this pull request Nov 17, 2020
* Add service generator

* Finalize service generator

* Add documentation

* Explicitly require rails/generators

* Update Zeitwerk initializer

* Remove explicit require

* Update docs/backend/service-objects.md

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants