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.

Sign up for GitHub

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

Jump to bottom

Throw when times-to-try > 1 and actual is a value #116

Merged
merged 3 commits into from
Apr 28, 2020
Merged

Throw when times-to-try > 1 and actual is a value #116

merged 3 commits into from
Apr 28, 2020

Conversation

dchelimsky
Copy link
Contributor

@dchelimsky dchelimsky commented Apr 27, 2020 •
edited
Loading

Users have reported failures when probing where it turned out that match? was given a value instead of a step to produce the value, e.g.

[res (do-a-step)]
(match? 37 res {:times-to-try 2})

This PR adds a warning when :times-to-try is > 1 and actual is a value instead of a step.

@dchelimsky dchelimsky requested review from sovelten, philomates and a team April 27, 2020 17:50
@dchelimsky dchelimsky changed the title WARN when times-to-match > 1 and actual is a value WARN when times-to-try > 1 and actual is a value Apr 27, 2020
@dchelimsky
Copy link
Contributor Author

I made a choice here to warn when running the flow. Other options include:

  • warn when compiling the flow
  • throw (not warn) when compiling the flow
  • throw (not warn) when running the flow

Anybody prefer any of those options? If so, why?

gabrielgiussi
gabrielgiussi reviewed Apr 27, 2020
View reviewed changes
src/state_flow/assertions/matcher_combinators.clj Outdated
(when (and (> (:times-to-try ~params*) 1)
(not (state/state? ~actual)))
(log/warn (select-keys ~caller-meta [:line])
":times-to-try > 1 has no meaningful effect when 'actual' is a value."))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is obvious for those who are used to work with state-flow but should we add, "'actual' should be a flow" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about "actual should be a step or a flow when :times-to-try > 1"?

@sovelten
Copy link
Collaborator

sovelten commented Apr 27, 2020 •
edited
Loading

I don't see why someone would want to retry something that is always the same, so I think an exception makes sense here (on compiling flow)

@dchelimsky
Copy link
Contributor Author

I don't see why someone would want to retry something that is always the same, so I think an exception makes sense here (on compiling flow)

I agree. My concern is that this is a breaking change for tests that are unnecessarily using :times-to-try. WDYT?

@sovelten
Copy link
Collaborator

sovelten commented Apr 27, 2020 •
edited
Loading

I don't see why someone would want to retry something that is always the same, so I think an exception makes sense here (on compiling flow)

I agree. My concern is that this is a breaking change for tests that are unnecessarily using :times-to-try. WDYT?

Yeah. All the tests that explicitly use times-to-try really want it to be retried I think. I don't expect many (or at all) people using :times-to-try without need, but who knows. I don't see it as a breaking change.

@fernando-nubank
Copy link
Contributor

I agree with @sovelten, an exception seems a better approach here.

@dchelimsky
Copy link
Contributor Author

Macroexpansion-time exception it is. Please re-review @sovelten and @fernando-nubank

@dchelimsky dchelimsky force-pushed the dc/warn-on-misused-match branch 2 times, most recently from e5f1b2b to b2531b4 Compare April 27, 2020 20:07
@dchelimsky dchelimsky force-pushed the dc/warn-on-misused-match branch from b2531b4 to 014c300 Compare April 27, 2020 20:08
dchelimsky
dchelimsky commented Apr 27, 2020
View reviewed changes
test/state_flow/assertions/matcher_combinators_test.clj

(testing "with times-to-try > 1 and a value instead of a step"
(testing "throws"
(is (re-find #"actual must be a step or a flow when :times-to-try > 1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: custom exception catching because the top-level exception is CompilationError because we're throwing at macroexpansion time.

@dchelimsky dchelimsky changed the title WARN when times-to-try > 1 and actual is a value Throw when times-to-try > 1 and actual is a value Apr 27, 2020
@dchelimsky dchelimsky merged commit b55f368 into master Apr 28, 2020
@dchelimsky dchelimsky deleted the dc/warn-on-misused-match branch April 28, 2020 13:41
@dchelimsky
Copy link
Contributor Author

This turns out to be a breaking change for anybody upgrading from state-flow before 2.2.4 and using the old version of match?. The problem is that the default times-to-try on the old match? is 5. It is then exacerbated by producing a confusing error about eval'ing locals.

Options:

  1. don't throw (revert this)?
  2. ¯_(ツ)_/¯

/cc @nubank/eng-prod-test @sovelten

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabrielgiussi gabrielgiussi gabrielgiussi left review comments

@fernando-nubank fernando-nubank fernando-nubank approved these changes

@sovelten sovelten Awaiting requested review from sovelten

@philomates philomates Awaiting requested review from philomates

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dchelimsky @sovelten @fernando-nubank @gabrielgiussi