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

Rename RadicalGroup to SolvableRadical (the old name is still supported as an obsolete synonym) #4655

Merged
merged 3 commits into from
Sep 18, 2021

Conversation

ThomasBreuer
Copy link
Contributor

and added the treatment of RadicalGroup as an obsolescent variable

Note that there is the notion of radical (p-)subgroups of a group,
which means something completely different,
and that solvable radical seems to be the term used in the litarature.

(I guess that the name RadicalGroup had been chosen --a long time ago--
because Radical was too unspecific, and thus adding the suffix Group
provided at least some context.)

Please provide a short summary of this PR and its purpose here. E.g., does it add a new feature, and which? Does it fix a bug, and which? If there is an associated issue, please list it here.

Text for release notes

We track noteworthy changes as entries in the CHANGES.md file in the root directory of this repository. To help us decide whether and how to describe your PR, please do one of the following:

  1. If this PR shall not be mentioned in the release notes, write "none" here.
  2. If a brief note suffices, edit the title of this PR to be usable as entry in CHANGES.md, and write "see title" here (or if you have the required permissions, add the label release notes: use title to this PR and remove this section)
  3. If a longer note is needed, just write it here, ideally following the general style used in that file

Further details

If necessary, provide further details down here.

@ThomasBreuer ThomasBreuer added topic: documentation Issues and PRs related to documentation topic: library kind: general proposed change release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Sep 12, 2021
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.

Is there a need to tag the synonym RadicalGroup immediately as obsolete? I have packages and private code that install methods, and there are publications out that explicitly refer to RadicalGroup. We can slowly change the princip[al use over, but I don't see a benefit in marking RadicalGroup for deletion.

@ThomasBreuer
Copy link
Contributor Author

@hulpke Would it be sufficient to set the minimal level of InfoObsolete that triggers the message about the recommended function name to 2? (There seems to be confusion about the default level, see pull request #4654)

Actually, I see no reason to remove obsolete synonyms for names that are regarded as better, except if they are wrong. The situation is different for obsolete code, where one can argue that it would be a burden to maintain it further.

@hulpke
Copy link
Contributor

hulpke commented Sep 13, 2021

As long as RadicalGroup continues to work and does not print warning or error messages I have no objection.

Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

When compiling the reference manual with this PR, there is a warning about an unresolved reference to RadicalGroup, which therefore needs to be updated to SolvableRadical. I'm not sure where the broken reference is.

#W WARNING: non resolved reference: rec(
  Attr := "RadicalGroup" )

lib/grp.gd Show resolved Hide resolved
lib/obsolete.gd Outdated
## </Description>
## </ManSection>
##
## 'RadicalGroup' was documented until version 4.11.1.
Copy link
Member

Choose a reason for hiding this comment

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

It should still be documented! Normally when we rename things and make the old name obsolete, then we add an entry for it (including an index entry) to doc/ref/obsolete.xml, and I think that should be done here.

So I'd rather say:

Suggested change
## 'RadicalGroup' was documented until version 4.11.1.
## 'RadicalGroup' was renamed in GAP 4.12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make this change.
But my understanding of the word "documented" is a positive one in the sense that one can rely on documented variables, and using undocumented variables can be risky. This was also the idea behind the (undocumented) function IsDocumentedWord; having index entries for deprecated variable names causes that the current implementation of this function does not fit to its intended meaning.

Concerning the sentence "Normally ...":
Currently we do not claim to have a complete list of name changes in the chapter called "Replaced and Removed Command Names" in the Reference Manual. The section about group actions lists "some examples of such name changes", and the section "Miscellaneous Name Changes or Removed Names" states that it lists "some further name changes". I think it would be better to have a complete list, and to state this --a topic for a new issue.

lib/obsolete.gd Outdated Show resolved Hide resolved
@ThomasBreuer
Copy link
Contributor Author

@wilfwilson Thanks for the hint. This is very strange, in fact many instances of RadicalGroup were not replaced by the pull request; I do not understand what happened, but I will try to update the pull request in order to fix this problem.

and added the treatment of `RadicalGroup` as an obsolescent variable

Note that there is the notion of radical (p-)subgroups of a group,
which means something completely different,
and that solvable radical seems to be the term used in the litarature.

(I guess that the name `RadicalGroup` had been chosen --a long time ago--
because `Radical` was too unspecific, and thus adding the suffix `Group`
provided at least some context.)
I do not understand how these instances managed to escape from the first commit.
@fingolfin fingolfin merged commit 1ec559f into gap-system:master Sep 18, 2021
@fingolfin fingolfin changed the title renamed RadicalGroup to SolvableRadical Rename RadicalGroup to SolvableRadical (the old name is still supported as an obsolete synonym) Aug 17, 2022
@fingolfin fingolfin added kind: removal or deprecation A feature was removed or deprecated / made obsolete and removed kind: general proposed change topic: documentation Issues and PRs related to documentation labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: removal or deprecation A feature was removed or deprecated / made obsolete 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants