Skip to content

Test cycle #773

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
Merged
merged 1 commit into from
Nov 1, 2025
Merged

Test cycle #773

merged 1 commit into from
Nov 1, 2025

Conversation

@super7ramp
Copy link
Contributor

@super7ramp super7ramp commented Oct 30, 2025

This closes #207.

@jeaye jeaye requested a review from E-A-Griffin October 31, 2025 18:05
jeaye
jeaye requested changes Oct 31, 2025
View reviewed changes
Copy link
Member

@jeaye jeaye 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 the PR!

test/clojure/core_test/cycle.cljc Outdated
Comment on lines 21 to 23
(are [expr] (thrown? #?(:cljs js/Error :default Exception) (expr))
(#'cycle)
(#'cycle [] [])))))
Copy link
Member

@jeaye jeaye Oct 31, 2025

Choose a reason for hiding this comment

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

Arity checking is not handled by the function definition. It's handled by the runtime. We don't want to include calls to unsupported arities in our unit tests here, since it's not actually testing the function.

Copy link
Contributor Author

@super7ramp super7ramp Oct 31, 2025

Choose a reason for hiding this comment

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

Removed tests on bad arities.

test/clojure/core_test/cycle.cljc Outdated
1 '() []
1 '(1 2 3) [1]
3 '(1 2 3) [1 2 3]
7 '(1 2 3) [1 2 3 1 2 3 1]))
Copy link
Member

@jeaye jeaye Oct 31, 2025

Choose a reason for hiding this comment

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

We can test infinite seqs here, too, since cycle needs to work with them. For example, (take 3 (cycle (range))).

Copy link
Contributor Author

@super7ramp super7ramp Oct 31, 2025

Choose a reason for hiding this comment

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

Added a test on infinite seq.

@jeaye jeaye mentioned this pull request Oct 31, 2025
Merged
E-A-Griffin
E-A-Griffin reviewed Oct 31, 2025
View reviewed changes
Copy link
Collaborator

@E-A-Griffin E-A-Griffin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could we add some tests for other data structures since the docs for this specifically only require a collection (e.g. range, set, map, at least one associative collection besides vector, doesn't need to be all of them)

@super7ramp
Copy link
Contributor Author

super7ramp commented Oct 31, 2025

Could we add some tests for other data structures since the docs for this specifically only require a collection (e.g. range, set, map, at least one associative collection besides vector, doesn't need to be all of them)

Added cases for set and map.

@jeaye jeaye merged commit d575dc6 into jank-lang:main Nov 1, 2025
2 checks passed
@jeaye
Copy link
Member

jeaye commented Nov 1, 2025

Well done!

@super7ramp super7ramp deleted the cycle branch November 1, 2025 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeaye jeaye jeaye requested changes

+1 more reviewer

@E-A-Griffin E-A-Griffin E-A-Griffin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

clojure.core/cycle

3 participants

@super7ramp @jeaye @E-A-Griffin