Remove tabs and unused local variables (DONE) #5275

james-d-mitchell · 2022-12-15T10:48:34Z

This PR removes all tabs and unused local variables from files starting "c" in the library. There were no tabs and no unused local variables in files starting "b" in the library, hence the lack of a PR for these.

Text for release notes

None

fingolfin · 2022-12-15T11:12:20Z

For the sake of efficiency, and to avoid having 20+ "convert tab commits" in the history and the .git-blame-ignore-revs, we could also have multiple of these "convert tabs" commits in a single PR; then we can review commit-by-commit to double check. And once all commits are "there" and reviewed, we could squash-merge.

This might also be easier for you compared to making many separate PRs?

That said, I am totally fine either way

james-d-mitchell · 2022-12-15T11:13:51Z

I can do that if it's preferable, I thought you'd asked for a number of smaller PR's rather than one massive one, but perhaps I misunderstood.

james-d-mitchell · 2022-12-17T18:10:30Z

I think it might take me a little while to do the rest of the files in the library, and it might be easier to merge this PR sooner rather than later (if it's acceptable) since otherwise the PR and master will diverge and become harder to merge.

fingolfin

Some minor oddities... no worries, we'll figure them out and then can merge (squash-merge?) this PR soonish. Need to sleep now, though

lib/clashom.gi

lib/clas.gi

lib/clashom.gi

james-d-mitchell · 2022-12-22T11:29:09Z

@fingolfin this is now complete, I've removed all unused local variables and tabs from lib/*.

ChrisJefferson

I had a quick look through and these all seem sensible. I looked closely at a few places where local variables had very suspicious names (like PrintObj or Error), but these variables were never used for anything, so removing them is safe (and good).

james-d-mitchell · 2022-12-22T13:20:16Z

Thanks @ChrisJefferson, I was going to say: I'll squash the commits which remove tabs into one, and the same for the commits removing unused local variables, if this PR gets the green light.

fingolfin · 2022-12-22T13:24:06Z

Thanks I'll have a look in a bit, too.

I am slightly vary about the files in hpcgap/lib/ getting out of sync. Perhaps there aren't too many tabs in them, though?

fingolfin · 2022-12-22T13:25:18Z

Thanks I'll have a look in a bit, too.

I am slightly vary about the files in hpcgap/lib/ getting out of sync. Perhaps there aren't too many tabs in them, though?

james-d-mitchell · 2022-12-22T13:28:24Z

Good point @fingolfin, there are apparently 167 lines with tabs in hpcgap/lib and a grand total of two local variables declared but not used.

fingolfin

I've checke a-e, all good, some very minor suggestions for improvements. I'll look at the rest later.

lib/clashom.gi

lib/claspcgs.gi

lib/ctbllatt.gi

lib/cyclotom.gi

fingolfin

Done a-f

lib/factgrp.gi

lib/fastendo.gi

lib/fpmon.gd

lib/fpsemi.gi

james-d-mitchell · 2022-12-22T17:41:24Z

Ugh, and I just accidentally force pushed and removed the changes that I merged on GitHub directly, I'll fix those again, sorry.

Also the latest commit in this PR removes some variables that are assigned but never used. I'm happy to put this in a separate PR if that's better, this change might be more problematic than the removal of unused local variables if the assignment of the variables has side effects. I've only removed those assigned but unused variables where I was fairly sure there'd be no side effects, in the other cases, I just remove the variable but left the right hand side of the assignment with a comment asking if the line is really required.

fingolfin

Done until and including grppccom.gi

A bunch of minor remarks; many are about formatting issues that were present before

lib/galois.gi

lib/ghomfp.gi

lib/gprdperm.gi

lib/grppc.gi

lib/grppcaut.gi

fingolfin

Done a-l

lib/grppcfp.gi

lib/grppcnrm.gi

fingolfin

Completed a-o

lib/mapprep.gi

lib/meatauto.gi

lib/mgmcong.gi

lib/mgmhom.gi

lib/mgmideal.gi

lib/numtheor.gi

lib/oprtpcgs.gi

ChrisJefferson · 2022-12-23T16:37:36Z

If this PR is squashed (not saying it has to be), could it be put into at least 3 commits?

Whitespace,
Remove variables which are never used,
Remove variables which were assigned, but then never used.

james-d-mitchell · 2022-12-23T16:57:02Z

@ChrisJefferson yes, I'll do that once everything is ok

james-d-mitchell · 2022-12-23T17:39:01Z

Thanks for the comments @fingolfin, I imagine it was as interesting to look at the changes in these files as it was to make them :) I think I've addressed all of your comments now, please let me know when you're done, so I can squash the commits according to what @ChrisJefferson suggested.

fingolfin

covered a-z now

lib/pcgsperm.gi

lib/randiso.gi

lib/relation.gi

lib/stbcbckt.gi

lib/streams.gi

lib/tietze.gi

lib/twocohom.gi

fingolfin · 2023-01-04T22:42:00Z

@james-d-mitchell how shall we proceed to get this merged? There are still comments by me (which I could apply, or not, or you; I am fine either way, just don't want to cause accidents).

Also, the linter CI actually fails?

fingolfin · 2023-01-04T22:42:50Z

.gaplint.yml

@@ -1,5 +1,4 @@
 disable:
- W000


I think these .gaplint.yml are not possible while there are still files in hpcgap/lib/ which are not de-tabbed

@james-d-mitchell how shall we proceed to get this merged? There are still comments by me (which I could apply, or not, or you; I am fine either way, just don't want to cause accidents).

Also, the linter CI actually fails?

Thanks @fingolfin I'll finish this off at the start of next week when back at work, sorry for the seasonal delay :)

@fingolfin I think I've now finished this in the sense that I've remove all tabs, unused local variables, and assigned but not used local variables in the library, and in hpc gap. The latter required some changes to gaplint which I released in v1.1.4 a couple of moments ago, although I pushed to this branch after the release it looks like it might take a little longer to become available, and I'd expect the "lint" job pass when gaplint v1.1.4 is available.

Looks like one of the other tests has failed but I don't think that's related to this PR, although I might be wrong.

The final thing I'll do once this has the green light is to squash into the 3 commits that @ChrisJefferson suggested.

I've restarted the gaplint job. If it finishes, please squash as described.

The testexpect failure is indeed separate, see issue #5309

Will squash now thanks!

Squashed, hopefully correctly.

fingolfin · 2023-01-10T08:47:31Z

lib/tietze.gi

@@ -656,7 +654,7 @@ InstallGlobalFunction( PresentationRegularPermutationGroupNC, function ( G )
                        if gen0[c] = 0 then
                            app[10] := g;
                            app[11] := c;
-                            n := MakeConsequences( app );
+                            MakeConsequences( app ); # TODO is this line required?


This line is definitely needed.

Suggested change

MakeConsequences( app ); # TODO is this line required?

MakeConsequences( app );

But fine by me to clean this up in a separate PR later (I can do it myself). Just wanted to mark it here so I don't forget.

(One CI test had a spurious failure due to a download timeout, I've just restarted it)

Thanks @fingolfin !

james-d-mitchell changed the title ~~Remove tabs c files~~ Remove tabs and unused local variables from "c" files Dec 15, 2022

fingolfin added topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Dec 15, 2022

james-d-mitchell marked this pull request as draft December 15, 2022 15:47

james-d-mitchell changed the title ~~Remove tabs and unused local variables from "c" files~~ WIP: Remove tabs and unused local variables (done files starting "a" to "c") Dec 15, 2022

james-d-mitchell changed the title ~~WIP: Remove tabs and unused local variables (done files starting "a" to "c")~~ Remove tabs and unused local variables (done files starting "a" to "h") Dec 17, 2022

james-d-mitchell marked this pull request as ready for review December 17, 2022 18:09

fingolfin reviewed Dec 18, 2022

View reviewed changes

lib/clashom.gi Outdated Show resolved Hide resolved

lib/clashom.gi Show resolved Hide resolved

lib/clashom.gi Outdated Show resolved Hide resolved

lib/clas.gi Outdated Show resolved Hide resolved

fingolfin reviewed Dec 19, 2022

View reviewed changes

lib/clashom.gi Outdated Show resolved Hide resolved

james-d-mitchell changed the title ~~Remove tabs and unused local variables (done files starting "a" to "h")~~ Remove tabs and unused local variables (done files starting "a" to "m") Dec 20, 2022

james-d-mitchell changed the title ~~Remove tabs and unused local variables (done files starting "a" to "m")~~ Remove tabs and unused local variables (done files starting "a" to "t") Dec 22, 2022

james-d-mitchell force-pushed the remove-tabs-c-files branch from ac7f153 to fea6037 Compare December 22, 2022 11:28

james-d-mitchell changed the title ~~Remove tabs and unused local variables (done files starting "a" to "t")~~ Remove tabs and unused local variables (DONE) Dec 22, 2022

ChrisJefferson approved these changes Dec 22, 2022

View reviewed changes

fingolfin reviewed Dec 22, 2022

View reviewed changes

james-d-mitchell force-pushed the remove-tabs-c-files branch from 742884b to cb55a6a Compare December 22, 2022 17:36

fingolfin reviewed Dec 22, 2022

View reviewed changes

fingolfin reviewed Dec 23, 2022

View reviewed changes

fingolfin reviewed Dec 24, 2022

View reviewed changes

fingolfin reviewed Jan 4, 2023

View reviewed changes

fingolfin mentioned this pull request Jan 9, 2023

Fix a rare error message in isomorphism test #5298

Merged

james-d-mitchell added 4 commits January 9, 2023 16:02

Remove tabs from files lib/*

9dc4ae1

Remove unused local variables from lib/*

a945372

Remove assigned but unused local variables from lib/*

5915ba1

ci: use gaplint v1.1.4 + enable more rules

20ebe5b

james-d-mitchell force-pushed the remove-tabs-c-files branch from 7746c76 to 20ebe5b Compare January 9, 2023 16:08

fingolfin approved these changes Jan 10, 2023

View reviewed changes

fingolfin enabled auto-merge (rebase) January 10, 2023 08:47

fingolfin merged commit 3d47e2b into gap-system:master Jan 10, 2023

james-d-mitchell deleted the remove-tabs-c-files branch January 10, 2023 09:48

james-d-mitchell mentioned this pull request Jan 11, 2023

Improvements to double cosets and minor additions #5308

Merged

fingolfin mentioned this pull request Jan 12, 2023

Recent changes cause test suites of groupoids, semigroups, xmod to fail #5318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove tabs and unused local variables (DONE) #5275

Remove tabs and unused local variables (DONE) #5275

james-d-mitchell commented Dec 15, 2022

fingolfin commented Dec 15, 2022

james-d-mitchell commented Dec 15, 2022

james-d-mitchell commented Dec 17, 2022

fingolfin left a comment

james-d-mitchell commented Dec 22, 2022

ChrisJefferson left a comment

james-d-mitchell commented Dec 22, 2022

fingolfin commented Dec 22, 2022

fingolfin commented Dec 22, 2022

james-d-mitchell commented Dec 22, 2022

fingolfin left a comment

fingolfin left a comment

james-d-mitchell commented Dec 22, 2022

fingolfin left a comment

fingolfin left a comment

fingolfin left a comment

ChrisJefferson commented Dec 23, 2022

james-d-mitchell commented Dec 23, 2022

james-d-mitchell commented Dec 23, 2022

fingolfin left a comment

fingolfin commented Jan 4, 2023

fingolfin Jan 4, 2023

james-d-mitchell Jan 5, 2023

james-d-mitchell Jan 9, 2023

fingolfin Jan 9, 2023

james-d-mitchell Jan 9, 2023

james-d-mitchell Jan 9, 2023

fingolfin Jan 10, 2023

james-d-mitchell Jan 10, 2023

	MakeConsequences( app ); # TODO is this line required?
	MakeConsequences( app );

Remove tabs and unused local variables (DONE) #5275

Remove tabs and unused local variables (DONE) #5275

Conversation

james-d-mitchell commented Dec 15, 2022

Text for release notes

fingolfin commented Dec 15, 2022

james-d-mitchell commented Dec 15, 2022

james-d-mitchell commented Dec 17, 2022

fingolfin left a comment

Choose a reason for hiding this comment

james-d-mitchell commented Dec 22, 2022

ChrisJefferson left a comment

Choose a reason for hiding this comment

james-d-mitchell commented Dec 22, 2022

fingolfin commented Dec 22, 2022

fingolfin commented Dec 22, 2022

james-d-mitchell commented Dec 22, 2022

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

james-d-mitchell commented Dec 22, 2022

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

ChrisJefferson commented Dec 23, 2022

james-d-mitchell commented Dec 23, 2022

james-d-mitchell commented Dec 23, 2022

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment