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 at_least_until to the maintain expectation #167

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

g-arjones
Copy link
Contributor

The plan is to add this to Syskit's have_no_new_sample helper if the concept and implementation are ok

@g-arjones g-arjones requested a review from doudou May 24, 2020 03:45
@doudou
Copy link
Member

doudou commented May 25, 2020

In principle, there's nothing wrong with it. I'd just want to be sure it's not something one can do already.

How is this at_least_until different from running two expectations in sequence, like:

expect_execution { ACTIONS }
   .to do
       maintain ....
       emit
    end

expect_execution.to do
    other_predicates_after_the_event_emission
end

@g-arjones
Copy link
Contributor Author

I guess it can be done with multiple expectations + execution settings (if one wants to run this during configure/start). It's syntactic sugar that I may be adding in the wrong place?

@doudou
Copy link
Member

doudou commented May 25, 2020

It's syntactic sugar that I may be adding in the wrong place?

Not necessarily. I'm really trying to think about it. It was rather a "what do you think" kind of question.

So, maybe the right answer is "let's try it out and see what happens later" :P

@g-arjones
Copy link
Contributor Author

Fair enough. I will add it in syskit and then we merge them both together.

@@ -1145,30 +1164,49 @@ def to_s
end

class Maintain < Expectation
def initialize(at_least_during, block, description, backtrace)
def initialize(at_least, block, description, backtrace)
Copy link
Member

Choose a reason for hiding this comment

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

You're making the difference between at_least and until in the user API (which is good) and then stop making it in this constructor to finally re-split on type...

Just propagate the same two arguments to the constructor.

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 didn't want to break the API

Copy link
Member

Choose a reason for hiding this comment

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

Please do, these are meant to be private. The public interface are the expectation API calls.

(and add a # @private documentation line with the Maintain class)

Comment on lines +1197 to +1198
emitted?(propagation_info) ||
(@at_least_during && Time.now > @deadline)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this never match if neither @at_least* are set?

I would check on @deadline here since the condition is using it.

Copy link
Contributor Author

@g-arjones g-arjones May 31, 2020

Choose a reason for hiding this comment

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

Doesn't this never match if neither @at_least* are set?

The user level API enforces one of them to be set (just as it did before this patch)

Copy link
Member

Choose a reason for hiding this comment

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

OK. Please add a comment explaining that, it will make things easier on the understanding.

Comment on lines +667 to +669
plan.add(generator = EventGenerator.new)
expect_execution { generator.emit }
.to { maintain(at_least_until: generator) { true } }
Copy link
Member

Choose a reason for hiding this comment

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

I would assume the caller would want the predicate to at least match once. Not sure what's your use-case though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Will fix.

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