Skip to content

Conversation

@ChrisJefferson
Copy link
Contributor

This is just a little tweak -- usually we pair enable/disable methods as: NameFunc and UnnameFunc, but TraceImmediateMethods is different, it takes a boolean flag.

This adds UntraceImmediateMethods, and lets TraceImmedaiteMethods take no flag (in which case it acts like it was passed true). We keep the old flag passing working, and also add a test.

stevelinton added a commit that referenced this pull request Nov 18, 2015
Add UntraceImmediateMethods, for name standardisation
@stevelinton stevelinton merged commit 1a646b8 into gap-system:master Nov 18, 2015
@ChrisJefferson ChrisJefferson deleted the trace-tweak branch November 18, 2015 09:10
@olexandr-konovalov
Copy link
Member

@ChrisJefferson Thanks - adding a new test file is more than just a little tweak :) if all packages are loaded the test fails. Probably some other example could be used?

########> Diff in /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGM\
P/gmp/GAPTARGET/install/label/graupius/GAP-git-snapshot/tst/testinstall/trace.\
tst, line 4:
# Input is:
g:= Group( (1,2,3), (1,2) );;
# Expected output:
#I  immediate: Size
#I  immediate: IsCyclic
#I  immediate: IsCommutative
#I  immediate: IsTrivial
# But found:
#I  immediate: MaintainanceMethodForToDoLists
#I  immediate: Size
#I  immediate: IsCyclic
#I  immediate: IsCommutative
#I  immediate: IsTrivial
#I  immediate: IsSemigroupIdeal
########
########> Diff in /home/hudson/hudson/workspace/GAP-dev/GAPCOPTS/64build/GAPGM\
P/gmp/GAPTARGET/install/label/graupius/GAP-git-snapshot/tst/testinstall/trace.\
tst, line 12:
# Input is:
g:= Group( (1,2,3), (1,2) );;
# Expected output:
#I  immediate: Size
#I  immediate: IsCyclic
#I  immediate: IsCommutative
#I  immediate: IsTrivial
# But found:
#I  immediate: MaintainanceMethodForToDoLists
#I  immediate: Size
#I  immediate: IsCyclic
#I  immediate: IsCommutative
#I  immediate: IsTrivial
#I  immediate: IsSemigroupIdeal
########

@ChrisJefferson
Copy link
Contributor Author

Actually no, this can't be fixed. If we want tests to work with RunAllPackages, I'll just have to delete the test, and hope we don't break this. What would you prefer?

@olexandr-konovalov olexandr-konovalov added this to the GAP 4.8.1 milestone Nov 20, 2015
@olexandr-konovalov
Copy link
Member

@ChrisJefferson oh, I see - MaintainanceMethodForToDoLists (actually with a typo in Maintenance) is a property (though its name is not beginning with Is) from the homalg project, declared for IsObject with an immediate method for object in IsAttributeStoringRep. Namely, this is what it does:

InstallImmediateMethod( MaintainanceMethodForToDoLists,
                        IsAttributeStoringRep,
                        0,
  function( obj )
    if TODO_LISTS.activated then
        SetFilterObj( obj, CanHaveAToDoList );
    fi;
    TryNextMethod();
end );

So because it is so generic, no other example would work... Perhaps @mohamed-barakat and @sebasguts could comment on this? If this has to be like this, then just delete this from the test
but put into manual example, BECAUSE manual examples are tested three times:

  • with no packages
  • with default packages
  • with all packages

and the build fails automatically if the 2nd option has diffs. In the other two diffs are permitted, but
at least there should be no crashes and errors. So the test of such example would be permitted to have diffs with homalg loaded, but at least it will be regularly tested.

This fact is good to remember for tests that may be very sensitive to some non-default packages.

@sebasguts
Copy link
Member

First of all, thanks for pointing out the typo. I will change this now. Now to the point:

The method is of actual use, and for the ToDoList feature in ToolsForHomalg it is necessary to have it. However, if that would be desireable, I could get rid of it with some work. Is it forbidden to implement an ImmediateMethod for all IsAttributeStoringRep ? If yes, please add some section in the manual, so that I remember it the next time I want to do something like that. If not, then the test should be moved like @alex-konovalov suggested, because some situation like this might happen again.

@olexandr-konovalov
Copy link
Member

@sebasguts thanks. Then I think that moving this to a manual example would be suitable for all.

@ChrisJefferson
Copy link
Contributor Author

Just to be clear, there is no reason to forbid this method, it just makes it hard to test tracing in our current tst framework (where we do exact text matching), with packages turned on.

@olexandr-konovalov
Copy link
Member

@ChrisJefferson I'm interested to fix this asap as I am getting now an avalanche of failed build notifications (24 for install and standard, plus another 12 for manuals because of changed random state) and would like to stop it before the next 6 pm nightly build). If you'd be happy for me to fix, please let me know!

@ChrisJefferson
Copy link
Contributor Author

I'm happy for you to fix, just delete for now and we will think about how to read later, is just a test.

@olexandr-konovalov
Copy link
Member

@ChrisJefferson done. Actually, the manual example for TraceImmediateMethods is already there, so it is covered by tests.

@sebasguts Please fix the typo in Maintenance - this will help to resolve ?Maintenance help queries correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants