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

Moved modules from Tasks to Tasks::Helpers #986

Closed
wants to merge 1 commit into from

Conversation

vimutter
Copy link
Contributor

@vimutter vimutter commented Jan 8, 2025

We ran into the issue that our AR only app stopped running specs due to check that Rails is >6/7.
First quick fix was not possible because tasks.rb contained both code and code execution.

  1. Moved module with methods to Helpers module. That way any adjustments on the logic could be done by:
require 'parallel_tests/tasks/helpers'
# monkey patch here 
require 'parallel_tests/tasks'
  1. Changed Rails check to actual ActiveRecord check since that's what DB stuff is about. Here I am suggesting fixing our original issue.

@vimutter vimutter force-pushed the patch-1 branch 2 times, most recently from 372ae64 to 162076d Compare January 8, 2025 18:17
@grosser
Copy link
Owner

grosser commented Jan 9, 2025

to monkey-patch the existing setup you can create the module, prepend, and then require

can you split the move from the change (make a new PR with just 1 or the other) ... hard to review otherwise 😞

@vimutter
Copy link
Contributor Author

vimutter commented Jan 9, 2025

Ah, never actually used prepend, thanks for the hint.

For whoever reading, here what is meant:

# Here we're in our patch code
module Patch
  def a
    print 'patched'
  end
end
module A
  class << self 
    prepend Patch
  end
end
# Patch code done, now we're loading original code
module A
  def self.a
    'not-patched'
  end
end
A.a # => 'patched'

Sure, will extract ActiveRecord change into separate PR.

@vimutter
Copy link
Contributor Author

vimutter commented Jan 9, 2025

Added #987

Please let me know if this PR still needed. I think it does, keeping code definition away from code execution seems right.

@grosser
Copy link
Owner

grosser commented Jan 9, 2025

afaik not needed, but also not against
... the code is not really executing when the require goes off, so 🤷

@grosser grosser closed this Jan 9, 2025
@grosser
Copy link
Owner

grosser commented Jan 9, 2025

v4.9.0

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