Improve and unify names (2) #277

ssiccha · 2021-06-18T16:13:38Z

This continues #253.

Renames as follows:
RIParent -> ParentRecogNode
methodsforfactor -> methodsforimage
forfactor -> InitDataForImageRecogNode
forkernel -> InitDataForKernelRecogNode

The documentation builds on my machine and I've checked a few cross-references and index entries. Everything looks fine there. testquick.g also passes on my machine.

Edit: I've moved the whole nice generators business into a separate branch.

codecov · 2021-06-18T16:24:55Z

Codecov Report

Merging #277 (6af8d45) into master (d8c21f1) will increase coverage by 0.00%.
The diff coverage is 71.05%.

❗ Current head 6af8d45 differs from pull request most recent head f462f18. Consider uploading reports for the commit f462f18 to get more accurate results

@@           Coverage Diff           @@
##           master     #277   +/-   ##
=======================================
  Coverage   77.85%   77.86%           
=======================================
  Files          43       43           
  Lines       18394    18398    +4     
=======================================
+ Hits        14321    14325    +4     
  Misses       4073     4073

Impacted Files	Coverage Δ
gap/matrix/matimpr.gi	`69.03% <7.69%> (ø)`
gap/projective/almostsimple.gi	`65.84% <20.00%> (ø)`
gap/projective/c6.gi	`62.17% <20.00%> (ø)`
gap/projective/tensor.gi	`56.33% <28.57%> (ø)`
gap/projective/c3c5.gi	`87.86% <75.00%> (ø)`
gap/matrix.gi	`94.19% <86.66%> (ø)`
gap/projective.gi	`83.09% <90.90%> (ø)`
gap/base/recognition.gd	`100.00% <100.00%> (ø)`
gap/base/recognition.gi	`69.02% <100.00%> (ø)`
gap/generic/KnownNilpotent.gi	`97.18% <100.00%> (ø)`
... and 2 more

FriedrichRober · 2021-06-18T16:41:52Z

I am also fine with calling the inner nodes of a recog tree node a "splitting node".

FriedrichRober · 2021-06-18T16:42:49Z

@ssiccha, there is a file howtowritearecogmethod.autodoc where the old names are still used, for example forfactor. Is this intended?

Edit: This is probably not intended, as this page is used in the documentation. Probably we do not grep this file in our renaming script.

FriedrichRober · 2021-06-18T16:54:27Z

Perhaps ParentRecogNode instead of ParentNodeOfRecogNode would be more consistent with the names KernelRecogNode and ImageRecogNode.

ssiccha · 2021-06-19T05:50:15Z

@ssiccha, there is a file howtowritearecogmethod.autodoc where the old names are still used, for example forfactor. Is this intended?

Edit: This is probably not intended, as this page is used in the documentation. Probably we do not grep this file in our renaming script.

I've updated our script. See here:
https://github.com/ssiccha/recog/tree/Improve-Function-Names

ssiccha · 2021-06-19T06:43:07Z

Perhaps ParentRecogNode instead of ParentNodeOfRecogNode would be more consistent with the names KernelRecogNode and ImageRecogNode.

Done.

ssiccha · 2021-06-19T07:23:58Z

Btw.: I'm seeing whether I can come up with better names for the whole CalcNice... business.

wilfwilson

Sergio! I'm happy with these four name changes, but you have been a bit overzealous with the search-and-replace of "factor" to "image".

contrib/akos/examples4.g

doc/recognition.xml

misc/obsolete/semilinear.gi

misc/steve/obrienmalle.gi

tst/working/quick/ProjSubfield2.tst

fingolfin · 2021-06-23T09:51:07Z

This needs to be rebased first?

wilfwilson

Thanks for making those fixes. But yes, it's rebasing time 🙂

ssiccha · 2021-06-28T19:40:10Z

Rebased onto master. Let's see whether the test workflows pass.

fingolfin · 2021-08-18T09:25:11Z

doc/renaming_index.xml

+<Index Key="RIParent"><C>RIParent</C></Index>
+<Index Key="methodsforfactor"><C>methodsforfactor</C></Index>
+<Index Key="forfactor"><C>forfactor</C></Index>
+<Index Key="forkernel"><C>forkernel</C></Index>


Not about this PR, but: why is there a separate index file? Why aren't those index entries directly in renaming_table.xml?

I changed our script such that the index is also written into the renaming_table.xml.

And I also tested whether the index still works. Everything is fine.

fingolfin · 2021-08-18T09:27:21Z

gap/base/recognition.gd

 DeclareAttribute( "KernelRecogNode", IsRecogNode, "mutable" );

-## <#GAPDoc Label="RIParent">
+## <#GAPDoc Label="ParentRecogNode">


These renames potentially break existing code using recog.

Is there a strong reason why we don't use DeclareObsoleteSynonym resp. DeclareObsoleteSynonymAttr for these (and other, previously renamed things)?

I added synonyms for the renames in this PR and am currently opening a PR for the renames that are already in master.

fingolfin · 2021-08-18T09:30:45Z

doc/recognition.xml


 <ManSection>
-<Attr Name="forkernel" Arg="ri"/>
+<Attr Name="InitDataForKernelRecogNode" Arg="ri"/>


I was confused by this name as to me init stands for the verb initialize, not for initial or initialization. And that makes for some confusing reading. So perhaps

Suggested change

<Attr Name="InitDataForKernelRecogNode" Arg="ri"/>

<Attr Name="InitialDataForKernelRecogNode" Arg="ri"/>

I changed the names accordingly.

`RIParent` -> `ParentRecogNode` `methodsforfactor` -> `methodsforimage` `forfactor` -> `InitialDataForImageRecogNode` `forkernel` -> `InitialDataForKernelRecogNode`

ssiccha · 2021-08-20T23:26:59Z

I've double-checked the touched files and the links in the generated manual. Everything looks fine so I'm merging this.

ssiccha added the enhancement label Jun 18, 2021

ssiccha requested review from FriedrichRober, fingolfin and wilfwilson June 18, 2021 16:13

ssiccha added a commit to ssiccha/recog that referenced this pull request Jun 18, 2021

History gap-packages#277

7a41e6e

ssiccha force-pushed the ss/improve-unify-names-2 branch from fde0c27 to 59ddac1 Compare June 19, 2021 06:24

ssiccha added a commit to ssiccha/recog that referenced this pull request Jun 19, 2021

History gap-packages#277

5772916

This comment has been minimized.

Sign in to view

ssiccha force-pushed the ss/improve-unify-names-2 branch 2 times, most recently from 45e7260 to 6bf24d2 Compare June 19, 2021 11:06

wilfwilson requested changes Jun 20, 2021

View reviewed changes

ssiccha force-pushed the ss/improve-unify-names-2 branch 4 times, most recently from 888e9d2 to 95fd513 Compare June 21, 2021 12:13

wilfwilson approved these changes Jun 23, 2021

View reviewed changes

ssiccha added a commit to ssiccha/recog that referenced this pull request Jun 28, 2021

History gap-packages#277

5b7687b

ssiccha force-pushed the ss/improve-unify-names-2 branch from 95fd513 to d03dd8c Compare June 28, 2021 19:34

ssiccha added a commit to ssiccha/recog that referenced this pull request Jun 28, 2021

History gap-packages#277

4e4099e

fingolfin reviewed Aug 18, 2021

View reviewed changes

ssiccha added a commit to ssiccha/recog that referenced this pull request Aug 20, 2021

History gap-packages#277

7736c94

ssiccha added a commit to ssiccha/recog that referenced this pull request Aug 20, 2021

History gap-packages#277

eea6865

ssiccha force-pushed the ss/improve-unify-names-2 branch from d03dd8c to b7e75da Compare August 20, 2021 10:55

Improve and unify names (2)

f462f18

`RIParent` -> `ParentRecogNode` `methodsforfactor` -> `methodsforimage` `forfactor` -> `InitialDataForImageRecogNode` `forkernel` -> `InitialDataForKernelRecogNode`

ssiccha force-pushed the ss/improve-unify-names-2 branch from b7e75da to f462f18 Compare August 20, 2021 11:00

ssiccha merged commit 0401141 into gap-packages:master Aug 20, 2021

ssiccha deleted the ss/improve-unify-names-2 branch August 20, 2021 23:27

	<Attr Name="InitDataForKernelRecogNode" Arg="ri"/>
	<Attr Name="InitialDataForKernelRecogNode" Arg="ri"/>

Improve and unify names (2) #277

Improve and unify names (2) #277

Uh oh!

Conversation

ssiccha commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

FriedrichRober commented Jun 18, 2021

Uh oh!

FriedrichRober commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FriedrichRober commented Jun 18, 2021

Uh oh!

ssiccha commented Jun 19, 2021

Uh oh!

ssiccha commented Jun 19, 2021

Uh oh!

This comment has been minimized.

ssiccha commented Jun 19, 2021

Uh oh!

wilfwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fingolfin commented Jun 23, 2021

Uh oh!

wilfwilson left a comment

Choose a reason for hiding this comment

Uh oh!

ssiccha commented Jun 28, 2021

Uh oh!

fingolfin Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

ssiccha Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

ssiccha Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

fingolfin Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

ssiccha Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

fingolfin Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

ssiccha Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

ssiccha commented Aug 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ssiccha commented Jun 18, 2021 •

edited

Loading

codecov bot commented Jun 18, 2021 •

edited

Loading

FriedrichRober commented Jun 18, 2021 •

edited

Loading