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

Fix Cycles((),[ ]) to correctly return [ ] #3903

Merged
merged 1 commit into from
Mar 2, 2020
Merged

Fix Cycles((),[ ]) to correctly return [ ] #3903

merged 1 commit into from
Mar 2, 2020

Conversation

laurentbartholdi
Copy link
Contributor

Description

There is a case of Emptyophobia with CyclesOp:

in the previous version, 'Cycles((),[])' triggers an error. It now correctly returns '[]'.

fingolfin
fingolfin requested changes Feb 25, 2020
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Bug fixes ideally really should include a test case that demonstrates the issue is gone, e.g. in tst/testinstall/perm.tst near the existing Cycles tests.

lib/oprt.gi Outdated Show resolved Hide resolved
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 25, 2020
fingolfin
fingolfin approved these changes Mar 2, 2020
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thank you! This looks good to me now!

Could you please rebase so that the (hopefully now fixed) tests get another chance to run?

(While at it, perhaps you can also squash the two commits into one)

@coveralls
Copy link

Coverage Status

Coverage increased (+5.0e-05%) to 84.779% when pulling 27e9334 on laurentbartholdi:fix-cyclesop into b068bb1 on gap-system:master.

@fingolfin fingolfin merged commit c80a66f into gap-system:master Mar 2, 2020
@danielrademacher danielrademacher self-assigned this Feb 16, 2021
@danielrademacher danielrademacher changed the title Fixed bug with Cycles((),[]) Bug Fix: 'Cycles((),[])' now correctly returns '[]' Feb 16, 2021
@danielrademacher danielrademacher changed the title Bug Fix: 'Cycles((),[])' now correctly returns '[]' Bug Fix: 'Cycles(( ),[ ])' now correctly returns '[ ]' Feb 16, 2021
@danielrademacher danielrademacher removed their assignment Feb 16, 2021
@danielrademacher danielrademacher added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 16, 2021
@olexandr-konovalov olexandr-konovalov mentioned this pull request Feb 19, 2021
Closed
@olexandr-konovalov olexandr-konovalov mentioned this pull request Mar 23, 2021
Merged
@wilfwilson wilfwilson changed the title Bug Fix: 'Cycles(( ),[ ])' now correctly returns '[ ]' Cycles((),[ ]) now correctly returns [ ] Jun 10, 2021
@fingolfin fingolfin changed the title Cycles((),[ ]) now correctly returns [ ] Fix Cycles((),[ ]) to correctly return [ ] Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@laurentbartholdi @coveralls @fingolfin @danielrademacher