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 super advice for hooks #35

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Oct 29, 2020

As suggested by @koic in rubocop/rubocop-minitest#111

@andyw8 andyw8 changed the title Add super note for hooks Add super note for hooks Oct 29, 2020
@andyw8 andyw8 changed the title Add super note for hooks Add super advice for hooks Oct 29, 2020
README.adoc Outdated
end
----

# good
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot to delete this.

README.adoc Outdated
def setup
stub_something
end
end
Copy link
Member

Choose a reason for hiding this comment

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

The bad case and the good case are the same. For POROs that are not inherited from Minitest::Test, it may be clearer to remove them from the example code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it.


def setup
do_something
super
Copy link
Member

Choose a reason for hiding this comment

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

IMO, as the example code, it may be better to set up the superclass first.

def setup
  super
  do_something
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. I've also added a teardown example as I believe that would typically be in the opposite order.

@@ -480,6 +480,47 @@ _ { raise_exception }.must_raise TypeError

Check the http://docs.seattlerb.org/minitest/Minitest/Expectations.html[Minitest::Expectations doc] for more information about its usage.

=== Hooks [[hooks]]
Copy link
Member

Choose a reason for hiding this comment

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

It may be even clearer :-)

Suggested change
=== Hooks [[hooks]]
=== Lifecycle Hooks [[lifecycle-hooks]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that, but the Minitest docs seem to indicate that 'lifecycle hooks' are things such as before_setup and after_teardown, which are "meant for library writers, NOT for regular test authors."

https://www.rubydoc.info/gems/minitest/5.9.1/Minitest/Test/LifecycleHooks

Copy link
Member

Choose a reason for hiding this comment

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

I get it. Thank you for the explanation!

@andyw8 andyw8 force-pushed the andyw8/add-hooks-super-note branch from a6cd680 to 535301b Compare October 30, 2020 00:42
@andyw8 andyw8 force-pushed the andyw8/add-hooks-super-note branch from 535301b to b348034 Compare October 30, 2020 00:44
@koic koic merged commit f0361ff into rubocop:master Oct 30, 2020
@koic
Copy link
Member

koic commented Oct 30, 2020

Thank you!

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.

3 participants