Allow ignore offences using .isolator_todo.yml #13
Conversation
|
Thanks for contribution! I got the idea, that make sense. But I'm not sure the implementation is flexible enough and easy to use. Thus it would be easy to keep a TODO list for Isolator; and do not touch the codebase. I mean something like that: # .isolator_todo.yml
# name of the adapter
sidekiq:
- app/models/user.rb:48
# masks support too
- app/models/**/*.rbThat would require a more complex What do you think? |
|
That was my plan for the next PR :) Will come up with solution in a couple of days |
DmitryTsepelev
changed the title
|
It took a bit longer then I've expected, but I think I'm done with the very basic implementation. It seems to pass specs, but there are some space for improvements:
@palkan could you please review and let me know if I'm thinking in the right direction? Thanks |
lib/isolator/isolate.rb
Outdated
|
|
||
| def ignore_in_files(adapter, ignored_files) | ||
| adapter.ignore_if do | ||
| caller_paths = caller.map { |row| row.split(":in").first } |
There was a problem hiding this comment.
What about using Regex here (build from ignore pattern) to avoid splitting?
Then we could build the same ignore_if block for both exact lines and masks.
lib/isolator/isolate.rb
Outdated
| def isolate(id, **options) | ||
| raise "Adapter already registered: #{id}" if Isolator.adapters.key?(id.to_s) | ||
| adapter = AdapterBuilder.call(options) | ||
| register_todos(id.to_s, adapter) if isolator_todos |
There was a problem hiding this comment.
What about extracting all the ignoring logic into a separate module? And then:
Ignorer.configure(adapter)
lib/isolator/isolate.rb
Outdated
| end | ||
|
|
||
| def ignore_line(adapter, line) | ||
| full_line = Rails.root.join(line).to_s |
There was a problem hiding this comment.
We cannot use Rails here, Isolator is not Rails-specific.
Looks like we need a way to specify the path to the configuration somehow, e.g. Isolator.load_ingore_config "./.isolator_blabla.yml".
That also means that we cannot apply ignores within isolate method, since it's called when gem is loading.
So, we should configure all the ignores in a separate step (we can access existing adapters through Isolator.adapters).
To make it easier to use with Rails we can add a Railtie with an initializer to load configuration form Rails.root.join(TODO_CONFIG_PATH).
|
Thanks for the review! I think I've addressed all the issues, please let me know if I'm using the proper place to load the config for Rails app |
|
@palkan pinging again, probably there were no notification since I haven't mentioned you in the previous comment :) |
README.md
Outdated
| @@ -145,6 +145,17 @@ Isolator.adapters.sidekiq.ignore_if { Thread.current[:sidekiq_postpone] } | |||
|
|
|||
| You can add as many _ignores_ as you want, the offense is registered iff all of them return false. | |||
|
|
|||
| ### Using with legacy codebases | |||
There was a problem hiding this comment.
Let's split the instructions into two parts: "Using with Rails" and "Using with Ruby".
We need an example on how to use ignore config in non-Rails apps too.
README.md
Outdated
|
|
||
| ``` | ||
| sidekiq: | ||
| - app/models/user.rb:20 |
There was a problem hiding this comment.
Let's add a wildcard example too (i.e. app/models/somthing/**/*.rb)
|
@palkan I think I'm done |
I'm trying to use Isolator on my current project and found the place where Sidekiq job is initiated from the
after_save. I'm planning to fix it a bit later, so I'd like to silence the offence caused by the exact line, for instance:Isolator.adapters.sidekiq.ignore_at("app/models/user.rb:48")