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

Reimplementing RangeGenerator #1526

Merged
merged 1 commit into from
Feb 23, 2019
Merged

Reimplementing RangeGenerator #1526

merged 1 commit into from
Feb 23, 2019

Conversation

rick-de-water
Copy link
Contributor

Description

The RangeGenerator was lost during the refactor of the generator interface. I've re-implemented it using the new generator interface. I have also extended it by adding an optional step size.

If it is satisfactory then I will update the documentation as well.

GitHub Issues

Disappeared in #1516

@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

Merging #1526 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1526   +/-   ##
=======================================
  Coverage   80.58%   80.58%           
=======================================
  Files         121      121           
  Lines        3383     3383           
=======================================
  Hits         2726     2726           
  Misses        657      657

@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

Merging #1526 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1526   +/-   ##
=======================================
  Coverage   80.54%   80.54%           
=======================================
  Files         121      121           
  Lines        3386     3386           
=======================================
  Hits         2727     2727           
  Misses        659      659

@@ -349,6 +349,53 @@ namespace Generators {
);
}

template <typename T>
class RangeGenerator final : public IGenerator<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it should be limited only for arithmetic types.

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 personally prefer to keep it open for anything that provides the correct interface. This way someone can still use it with their own classes, such as a BigInteger. A rare use case, but I think trying to prevent someone from putting a string in it isn't going to help anyone either.

But that's my opinion, if you really want me to I can always add a static_assert with std::is_arithmetic.

@JoeyGrajciar
Copy link
Contributor

Add also a documentation and example for this.

@rick-de-water
Copy link
Contributor Author

I have removed the possibility to use anything other than integers as requested.
The documentation has been updated as well.

@horenmar
Copy link
Member

I am back.

Apart from the two lines with hard tabs, this looks good and definitely mergeable. However, I remember from Discord that you wanted the RangeGenerator to work with non-integral types, while I would really rather it didn't (because the step can be hard to define, or even plain wrong, for non-integral types, such as float).

I think we can compromise by forcing the helper to do the checking for integralness of the types -- doing so would ensure that users wouldn't be bitten by something like range(-2.0, 7.4, 0.1), while they would be able to say "trust me, I know what I am doing", and construct RangeGenerator directly, with an arbitrary type.

@horenmar
Copy link
Member

I made the mentioned changes and squashed the PR together, gonna merge it later after it goes through CI.

@horenmar horenmar merged commit 165de9b into catchorg:master Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants