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

Refactor test/ranges.jl range construction #39806

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Feb 24, 2021

Refactor test/ranges.jl "range construction" testsets

Over several pull requests the inner @testset "range(;kw...)" in test/ranges.jl introduced in #38041 by @jw3126 to test the all keyword form of range was lost. Here we reintroduce it and add separate test sets for range(start, stop) and range(start, stop, length).

Let block and compact keyword syntax

The tests are also reformatted to use a let block rather than repeatedly calling first, last, step, and length. The let block also allows use to use the compact keyword calling syntax when the variable name matches the keyword name. Alignment is kept across the test sets.

Additional malformed calls of ranges

Additionally, several malformed calls of range are added based on other pull requests where such forms were proposed and found to be problematic or otherwise declined.

Summary

In summary, the refactor results in the following form with distinct tests groups for the range(; kw...), range(start, stop), and range(start, stop, length).

@testset "range construction" begin
    @testset "range(; kw...)" begin
        ...
        let start=first(r), stop=last(r), step=step(r), length=length(r)
            @test r === range(; start, stop, step        )
            @test r === range(; start, stop,       length)
            ...
        end
    end
    @testset "range(start, stop)" begin
        ...
        let start=first(r), stop=last(r), step=step(r), length=length(r)
            q = range(start, stop)
            ...
            @test q === range(  start;            length)
            ...
        end
    end
    @testset "range(start, stop, length)" begin
        ...
    end
end

History:

  1. Extracted from range(; stop) and range(; length). Single keyword args only. Single pos arg not allowed. #39241 and related feature branches. This pull request only add tests and no new features.

@mkitti mkitti force-pushed the range_test_refactor branch from e9a437d to 3229823 Compare February 24, 2021 06:56
test/ranges.jl Outdated
@test q === range(; start, stop )
@test q == range(; start, stop, step )
@test q == range(; start, stop, length)
@test q ≈ range(; stop, length)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add @test_broken q == range(; stop, length)? Not sure, equality seems very hard to achieve here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually on master I currently get equality:

julia> r = 2.8:20.0
2.8:1.0:19.8

julia> using Test

julia> let start=first(r), stop=last(r), step=step(r), length=length(r)
           q = range(start, stop)
           @test q == range(; stop, length)
       end
Test Passed

julia> versioninfo()
Julia Version 1.7.0-DEV.551
Commit 9daf90ab49* (2021-02-16 19:40 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.1 (ORCJIT, skylake)

Copy link
Contributor

@jw3126 jw3126 left a comment

Choose a reason for hiding this comment

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

Thanks for this and all the energy you put into ranges. I have a package https://github.com/jw3126/RangeHelpers.jl where I experiment with ways to make range construction even more convenient. Would be interested in your opinion.

@mkitti
Copy link
Contributor Author

mkitti commented Feb 24, 2021

I have a package https://github.com/jw3126/RangeHelpers.jl

This looks interesting. You should be able to extend Base.range for your Approach type rather than duplicating range in your package. I'll take a deeper look and comment over there.

@mkitti mkitti force-pushed the range_test_refactor branch from 8cc86b8 to e32fda2 Compare March 28, 2021 04:44
@mkitti
Copy link
Contributor Author

mkitti commented Mar 28, 2021

Rebased

@mkitti
Copy link
Contributor Author

mkitti commented Oct 27, 2023

I'm abandoning this. I think some similar changes may have been merged already. I will close this shortly after reviewing if any changes should be resubmitted.

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