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

Use Pluralize to give correct pluralization in many GAP library methods #4050

Merged
merged 1 commit into from
Jul 2, 2022
Merged

Use Pluralize to give correct pluralization in many GAP library methods #4050

merged 1 commit into from
Jul 2, 2022

Conversation

wilfwilson
Copy link
Member

@wilfwilson wilfwilson commented Jun 11, 2020 •
edited by fingolfin
Loading

This PR was originally part of #3992, so it has already been discussed there.

This PR uses the Pluralize function in many locations in the GAP library, so that GAP produces more grammatically correct strings - such as <magma with 1 generator> rather than <magma with 1 generators>. There is also a follow-up PR #4343.

This PR affects the ViewString of many objects, and so this will cause the tests/manual examples for many packages to fail. Therefore this cannot be merged until those tests/examples are adjusted, and new package versions are released. I will use this description to keep track of what needs to be done for that, initially copied from #3992 (comment).

Open PRs

None.

Pending release

None.

Resolved

@wilfwilson wilfwilson added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes priority: low labels Jun 11, 2020
@wilfwilson wilfwilson mentioned this pull request Jun 11, 2020
Merged
@coveralls

This comment has been minimized.

Sign in to view
@wilfwilson wilfwilson mentioned this pull request Jun 27, 2020
Merged
hulpke
hulpke approved these changes Jul 13, 2020
View reviewed changes
Copy link
Contributor

@hulpke hulpke 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 is a good idea.

This was referenced Jul 26, 2020
Closed
Merged
Merged
Merged
Closed
Merged
Merged
This was referenced Oct 21, 2020
Merged
Merged
Closed
Merged
Merged
@wilfwilson wilfwilson mentioned this pull request Oct 29, 2020
Merged
@wilfwilson wilfwilson mentioned this pull request Dec 16, 2020
Merged
@olexandr-konovalov
Copy link
Member

@wilfwilson I merged in Circle, LAGUNA and Wedderga - will try to make releases after switching them to GitHub actions for CI.

@wilfwilson wilfwilson force-pushed the pluralize branch 2 times, most recently from 41cedad to a53288a Compare February 22, 2021 08:53
This was referenced Mar 25, 2021
Open
Merged
@wilfwilson wilfwilson force-pushed the pluralize branch 2 times, most recently from 33dfc9f to c1e8d0a Compare March 29, 2021 07:10
@gap-system gap-system deleted a comment from codecov bot Apr 15, 2021
@wilfwilson wilfwilson mentioned this pull request Apr 28, 2021
Merged
@wilfwilson wilfwilson mentioned this pull request Aug 4, 2021
Merged
@wilfwilson
Use Pluralize in some methods
35b0720 
The inspiration for this was to avoid many occurrences
of the phrase "1 generators", which is incorrect
pluralisation (it should be "1 generator").
@fingolfin fingolfin mentioned this pull request Mar 29, 2022
Closed
@fingolfin
Copy link
Member

With the switch to the new package distro, we can merge this! (Well, ideally, we'd first run all package tests against this, using either the CI @wilfwilson set up at https://github.com/gap-infra/integration or the ones at https://github.com/gap-system/PackageDistro . But we don't have the tooling for that set up yet... but hopefully soon!

olexandr-konovalov
olexandr-konovalov approved these changes May 2, 2022
View reviewed changes
Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

Indeed, all checkboxes for GAP packages are now ticked.

@fingolfin fingolfin added this to the GAP 4.12.0 milestone Jul 1, 2022
@fingolfin fingolfin closed this Jul 1, 2022
@fingolfin fingolfin reopened this Jul 1, 2022
@fingolfin
Copy link
Member

@wilfwilson this PR still has a "do not merge" label. Can it be removed?

@wilfwilson
Copy link
Member Author

wilfwilson commented Jul 1, 2022 •
edited
Loading

Yes 🙂

@wilfwilson wilfwilson removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jul 1, 2022
@fingolfin fingolfin closed this Jul 2, 2022
@fingolfin fingolfin reopened this Jul 2, 2022
@fingolfin fingolfin merged commit 06bbef4 into gap-system:master Jul 2, 2022
@fingolfin
Copy link
Member

Unfortunately this broke the QPA tests, see https://github.com/gap-system/PackageDistro/runs/7161848758?check_suite_focus=true -- on the positive, side, it only broke those. Reported at gap-packages/qpa#71

@wilfwilson
Copy link
Member Author

I'm amazed it broke so little! Thank you for reporting that @fingolfin.

@wilfwilson wilfwilson deleted the pluralize branch July 2, 2022 15:57
@fingolfin fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use 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 Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hulpke hulpke hulpke approved these changes

@olexandr-konovalov olexandr-konovalov olexandr-konovalov approved these changes

Assignees
No one assigned
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements priority: low release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Milestone
GAP 4.12.0
Development

Successfully merging this pull request may close these issues.
5 participants
@wilfwilson @coveralls @olexandr-konovalov @fingolfin @hulpke