Skip to content

Add order objects - #474

Merged
james-d-mitchell merged 27 commits into
libsemigroups:mainfrom
james-d-mitchell:add-order-objects
Aug 20, 2026
Merged

Add order objects#474
james-d-mitchell merged 27 commits into
libsemigroups:mainfrom
james-d-mitchell:add-order-objects

Conversation

@james-d-mitchell

Copy link
Copy Markdown
Member

No description provided.

@james-d-mitchell james-d-mitchell added the libsemigroups-feature-not-yet-supported Label for issues and PRs related to features of libsemigroups not yet supported here label Aug 17, 2026

@Joseph-Edwards Joseph-Edwards left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A number of tweaks (mostly related to the doc and tests), but overall this looks really good! Thanks @james-d-mitchell

Comment thread docs/source/data-structures/order/index.rst Outdated
Comment thread docs/source/data-structures/order/functions.rst Outdated
Comment thread docs/source/data-structures/order/index.rst Outdated

"""Subpackage for importing and wrapping :any:`du_narendran_rusinowitch`."""

# pylint: disable=no-name-in-module

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think this should be necessary

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let's discuss this, I'm also not sure why this is necessary but without it I get a warning when running pylint locally.

Comment thread src/order.cpp Outdated
Comment thread src/order.cpp Outdated
Comment thread src/order.cpp Outdated
Comment thread src/order.cpp
Comment thread tests/test_order.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The lack of structure in this file makes it a bit difficult to check we're testing the API fully. It would be better if this file was split up into different sections per order, and within each section, there were tests for:

  • with alphabet
  • without alphabet
  • copy and repr (object only, not _cmp functions)
  • rejects bad constructor arguments (object only, not _cmp functions)

As far as I can tell, we aren't obviously missing anything, but I'm not confident in that.

Comment thread src/order.cpp
Comment thread src/order.cpp Outdated
Comment thread src/order.cpp Outdated
Comment thread src/order.cpp Outdated
Comment thread src/order.cpp Outdated
Comment thread src/order.cpp Outdated
@james-d-mitchell

Copy link
Copy Markdown
Member Author

@Joseph-Edwards thanks for the review, I think that should be everything now.

@james-d-mitchell
james-d-mitchell enabled auto-merge (squash) August 20, 2026 14:00
@james-d-mitchell
james-d-mitchell merged commit 44ed1c3 into libsemigroups:main Aug 20, 2026
33 of 34 checks passed
@james-d-mitchell
james-d-mitchell deleted the add-order-objects branch August 20, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libsemigroups-feature-not-yet-supported Label for issues and PRs related to features of libsemigroups not yet supported here

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants