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

Modernize RSpec configuration #16699

Merged
merged 8 commits into from
Feb 19, 2024
Merged

Conversation

dduugg
Copy link
Member

@dduugg dduugg commented Feb 18, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This PR enables some best-practice RSpec options (for reference, see the project initializer boilerplate in rspec-core). Specifically, it:

  • Enables and fixes warnings (first commit). These involved a circular require and re-defining methods without undefining them first. (The latter can seem a bit silly, but I've found to be very useful in understanding code on occasion.)
  • Enables and fixes verify_partial_double (second commit). This checks that the underlying object actually defines the doubled methods. This uncovered some cruft, though I had to use the escape hatch of without_partial_double_verification around ENV code that I couldn't make immediate sense of.
  • Enables and fixes disable_monkey_patching (remaining commits). This setting removes RSpec methods from the top-level scope.

@dduugg dduugg changed the title Modern rspec Modernize RSpec configuration Feb 18, 2024
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks great, thanks again @dduugg!

Copy link
Member

@MikeMcQuaid MikeMcQuaid 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, thanks @dduugg! Remaining feedback can be addressed in follow-up PR, let's get this merged before it conflicts.

@MikeMcQuaid
Copy link
Member

@dduugg Looks like some legit test failures post-rebase, sorry!

Copy link
Member

@reitermarkus reitermarkus left a comment

Choose a reason for hiding this comment

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

@dduugg, can you move your last commit to a separate PR? I couldn't really finish reviewing because the “Files changed” tab just hangs.

@dduugg dduugg dismissed reitermarkus’s stale review February 19, 2024 16:56

per above, "let's get this merged before it conflicts."

@MikeMcQuaid MikeMcQuaid merged commit aa92559 into Homebrew:master Feb 19, 2024
24 checks passed
@dduugg dduugg deleted the modern-rspec branch February 19, 2024 16:56
@reitermarkus
Copy link
Member

let's get this merged before it conflicts.

Merging this quickly is not a good reason to dismiss my request for being able to fully review this. I certainly can't review it any better now that it is merged.

Remaining feedback can be addressed in follow-up PR

I mean it's generally fine to make further changes to a future PR, but I was not even able to review this fully in order to provide this “remaining feedback”.

@MikeMcQuaid
Copy link
Member

Merging this quickly is not a good reason to dismiss my request for being able to fully review this.

You did provide a round of review here and your comments were addressed.

I certainly can't review it any better now that it is merged.

You can definitely review PRs post-merge, I do this all the time. If there's problems: they can be addressed in a future PR.

I mean it's generally fine to make further changes to a future PR, but I was not even able to review this fully in order to provide this “remaining feedback”.

Another maintainer (me) was able to review it before it was merged.

@reitermarkus
Copy link
Member

You can definitely review PRs post-merge

Sure, not if “Files changed” tab just hangs though, which is why I specifically requested to split this up.

At least the individual commits seem to work now, they were also bugged by not showing the + button for commenting last time.

@@ -213,7 +213,7 @@ def install
end
end

describe ExecShellMetacharacters do
::RSpec.describe ExecShellMetacharacters do
Copy link
Member

Choose a reason for hiding this comment

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

Can we unnest these instead?

@github-actions github-actions bot added the outdated PR was locked due to age label Mar 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants