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

Allow substitutions in count and delay #207

Merged
merged 1 commit into from
Mar 14, 2017
Merged

Allow substitutions in count and delay #207

merged 1 commit into from
Mar 14, 2017

Conversation

cdent
Copy link
Owner

@cdent cdent commented Mar 11, 2017

Since we know that count must always be an int and delay can be
either an int or float, the yaml data is passed through
replace_template and then cast to int or float explicitly.

This is different previous solution which tries to rely on the
casting happening in replace_template. With the new coerce style of
numbers-in-data handling being developed in #206 that strategy won't
work. In any case, this way is more contained and explicit.

Fixes #150

Since we know that count must always be an int and delay can be
either and int or float, the yaml data is passed through
replace_template and then cast to int or float explicitly.

This is different previous solution which tries to rely on the
casting happening in replace_template. With the new coerce style of
numbers-in-data handling being developed in #206 that strategy won't
work. In any case, this way is more contained and explicit.

Fixes #150
@cdent
Copy link
Owner Author

cdent commented Mar 11, 2017

@josdotso and @FND if you could check over this, that would be great.

@FND
Copy link
Collaborator

FND commented Mar 11, 2017

lgtm - though I'm not entirely sure what that test is really testing (likely due to lack of familiarity; I'd have to do a little more digging than I can afford right now).

More generally, I'm a little concerned about the amount of imperative (as opposed to declarative*) logic in this function (or perhaps in general? that's entirely unfounded though). However, pragmatically speaking, I doubt that's actually a problem in any tangible way.

* Perhaps there could be some sort of key-transformation mapping for substitutions?

@cdent
Copy link
Owner Author

cdent commented Mar 11, 2017

this function

Which one? The one that's been changed? I'm not sure at this stage of processing there's any other option than imperative. We have a value, we want to to replace_template on it, we then want to turn it into a number (or fail). This does exactly that.

(replace_template itself is pretty messy, but you should have seen it in very early versions...it's actually gotten better)

The tests are very hand wavey, they prove that the right result is happening, but don't necessarily provide clear insight into the process whereby that result is caused. It's certainly not a unit test by any stretch. But I couldn't be bothered.

@FND
Copy link
Collaborator

FND commented Mar 12, 2017

D'accord.

@cdent cdent merged commit bb7867a into master Mar 14, 2017
@cdent cdent deleted the fix-150 branch March 14, 2017 13:41
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