You write:

I am sure that this can be written much better by using the filters and RedispatchOnCondition, so the whole code would actually be readable.

I actually think that code becomes less readable, and harder to understand, when RedispatchOnCondition, is used a lot. It makes behavior of code non-local, i.e. to really understand what goes on, one has to combine code which possible sits in completely different files.

So, not writing it that way actually does more than making reviewing easier; it also later on makes maintanance easier.

Added a test file for pgroups. I specifically checked (by hand) that all situations are triggered at least once, but I feel that the number of examples is rather limited. As this is my very first test file, any advice would be welcome.

Note that there is one particular code that I could not trigger. This is grp.gi:267. The reason is that I do not know how to give an example of a group in GAP so that GAP would not know its generators....

Here is an example of a group without known generators:

gap> G:=CyclicGroup(IsFpGroup, 4); H:=CyclicGroup(2);
<fp group of size 4 on the generators [ a ]>
<pc group of size 2 with 1 generators>
gap> hom:=GroupHomomorphismByImages(G,H);
[ a ] -> [ f1 ]
gap> K:=Kernel(hom);
Group(<fp, no generators known>)
gap> # But GAP can compute some generators if we ask it to do so
gap> GeneratorsOfGroup(K);
[ a^-2 ]
gap> HasGeneratorsOfGroup(K);
true

The two methods are: one checks if the size is a p-power. The other checks if all the generators are of p-power order. The second works if we know the group is nilpotent.

The original algorithm did the following (in a very convoluted way): if we know the size, then it factors it. If we do not know the size but know that the group is nilpotent, then it checks for the orders of the generators. If we do not know any of these, then it checks if the group is Abelian and if yes, then goes for the generator method, if not, then computes the size and then follows from there.

I am happy to rewrite this, because I do not like it as it is. But I did not want to make too much of a change, so that is why I made it this way.

About the nilpotent test examples: those are for testing the new nilpotent code: if the order of the group is a prime power then it is nilpotent (as it is a p-group). Maybe such checks should rather go to another test file (say nilpotent.tst), but then a lot of other test cases should be added (for direct products, for permutation groups, etc), and I really did not want to do that....

It's perfectly fine to keep this to just one test file -- and this test file could certainly also contain some computations with non-pgroups and non-nilpotent groups. Think of the tests focused on the specific functions, not on the class of objects they are dealing with. And of course a user may invoke IsPGroup on a group which is not a p-group, perhaps not even a nilpotent group (after all, they do not know the answer yet, and want GAP to find out for them). And of course we we want IsPGroup to return a correct answe in all of these cases.

Also note that is is also perfectly fine to add a test file that is (for the time being) "incomplete" (i.e. if you were to add a nilpotent.tst, I wouldn't expect you to test everything in it in one go. Actually, I am not even sure what would go into such a file, it probably would have to be split up anyway, as there are so may things related to nilpotent groups in GAP and that could be potentially be tested

But anyway, I am not asking you to add one, no worries :-).

BTW, in the future, I expect we will have many more test files; perhaps even one for each function / operation / atribute, with a filename reflecting the name of the function be tested.

OK, if IsPGroupNilpotent is only called for nilpotent groups, then it is of course correct. Then perhaps rename it to IsPGroup_forNilpotent or so? (That's not strictly following our usual naming conventions, but since this is an internal function, not to be used by the user, I'd consider this a bonus, as it kind of warns the user not to use it. Come to think, perhaps IS_PGROUP_FOR_NILPOTENT would be even better, so that it does not show up in tab completion (then also IsPGroupSize could become IS_PGROUP_SIZE).

Ok, I have renamed the functions. I kept the size method to depend on the group rather than on the size, because it sets the PrimePGroup attribute, as well, and that belongs to the group.

I will add some more examples into the tst file, and then come back with it.

This looks very good now. I'll wait for your new tests, and also for Travis to run those tests, but I am hopeful we can merge this today (or whenever you get around to do the extra work on the tests).

Ok, I added some more examples into the test. Computed some SylowSubgroups, as well, to check if they automatically get the IsPGroup property.

I have one more question. According to the manual (and also the code) IsPGroup returns false for all infinite groups. Could then maybe the IsPGroup filter contain the IsFinite filter? So if somebody wants to write a code checking for IsPGroup, then it would get the proper rank.

One example is Socle in lib/grppc.gi:2560. This has only filter 43, but IsFinite and IsPGroup would have filter 56, which would move it above IsFinite and IsSolvableGroup (having rank 48).

Regarding IsPGroup and IsFinite: I see your point, but we should not do that in this PR. I suggest writing an email to the gap mailing list with that idea -- it seems logical to me, but maybe somebody else will spot an issue with it... But if it turns out ok, you could open a separate PR for it (or somebody else could).

@fingolfin Sure. Should I do anything else here?

I think this is basically ready to be merged, thank you!

Though perhaps you'd be willing to run one last "interactive rebase": The typo fixes could be squashed suitably; "Add more examples into tst" could be squashed into "Add test file for p-groups"; and "Rename functions" into "Remove code duplication" (and perhaps more, your choice)

@fingolfin Ok, I will try to do it, and if I fail, then I will just let you know. :-)

Ok, I think I have managed to rebase the commits and squash them a bit. Force pushed (SW pun intended) them just now.

Any reason you did not squash "Correct small typos" into "Enhance IsPGroup"?

Anyway, I think this is good to go. Great work! I'll wait a bit longer to see whether you want to do that last squash, but I'll just merge it soon (tomorrow at the latest)

No particular reason, apart from keeping the history of things, as it was commented in this PR. Anyway, I rebased it, force pushed it, checks are running now. Thank you.

Thanks a lot for your patience and hard work.

I'll try to get to your other PRs this week, too (of course if somebody else beats me to, I am also fine with that)