-
Notifications
You must be signed in to change notification settings - Fork 27
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
CI::Queue::Static and CI::Queue::File initialization is broken #42
Comments
I understand it might seem awkward, but it's on purpose. All
We could certainly offer some higher level methods for convenience.
Same here, I could check for that and raise a cleaner error. |
I totally see this API change as being a good thing. But do we need to pass the file or the list of test to the initialize of |
It actually is. TBH I'm not 100% happy with all this because I try to make those 3 class share the same interface but they work quite differently internally. For e.g. all_tests = [
OpenStruct.new(name: 'a'),
OpenStruct.new(name: 'b'),
OpenStruct.new(name: 'c'),
OpenStruct.new(name: 'd'),
]
queue = Static.new(%w(a b))
queue.populate(all_tests, &:name)
queue.poll do |test|
p test.name
end will print
But ultimately users shouldn't have to mess with those internals, you had to because of the bisect script in core, but I'm working on bringing it inside the gem: #43 |
After #34, you need to initialize the File and Static queue passing the file name or the test respectively and you also need to populate the queue with the list of tests. This seems contra-intuitive.
Maybe the initialize should only take the config and the populate the list of tests and add a
.populate
class method (or any other name) that does what the initialize used to do.Also those queues require a configuration object that is not needed in most of its use cases. Making it optional, or adding a high level method as
.populate
that creates the configuration object would be great.cc @casperisfine @wvanbergen @tjoyal
Nitpick:
The
@index
instance variable is only set after you call#populate
so if you don't you get a "non-method error fetch for nil" in theto_a
method and a ruby warning because the instance variable is not initialized. Maybe we should try to write warning free code so we could avoid this kind of error?The text was updated successfully, but these errors were encountered: