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

Add ShowDeclarationsOfOperation helper #3459

Merged

Conversation

fingolfin
Copy link
Member

When debugging issues with method installations for operations (including
attributes and properties), it can be helpful to find out what all
declarations of a given operation are. This helper function does just that.

The placement of this function in the manual could perhaps be better, I am open to suggestions, but generally feel that this could and should be improved in that whole chapter

This is a new feature and hence should be listed in the release notes.

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels May 17, 2019
@coveralls
Copy link

coveralls commented May 17, 2019

Coverage Status

Coverage decreased (-0.002%) to 85.167% when pulling 402644d on fingolfin:mh/ShowDeclarationsOfOperation into 87794e8 on gap-system:master.

@DominikBernhardt
Copy link
Contributor

Thanks, this is indeed a very helpful function!
I don't have anything against the placement in the manual, although one could also think about putting it closer to the explanation of operations, i.e. 5.5-2 after IsOperation? It's not ideal either, but maybe a user would rather search there?

Copy link
Contributor

@markusbaumeister markusbaumeister left a comment

Choose a reason for hiding this comment

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

This looks very helpful for debugging purposes.

Concerning the manual placement: I believe the whole section "Global Variables in the Library" seems to be mislabeled - I would not have (and still don't) associate "global variables" with methods like "DeclareCategory" or "TypeOfOperation". While @DominikBernhardt has a slightly better suggestion, putting it under the header "Function Types" also feels weird.

If I were to search for this functionality, I would naturally look for a chapter that explains "Operations in GAP" (or "Categories in GAP" for the "IsCategory"), ideally in a chapter whose name makes it clear that these are internal data structures or methods usually only used by developers. Unfortunately, such a chapter does not currently exist. Lacking it, I would ask whether this method would fit into chapter 7 ("Debugging"), since that seems to be one of the primary use cases I can discern.

@DominikBernhardt
Copy link
Contributor

I agree with @markusbaumeister that the header Function Types itself does not feel perfect. But we have already established that there is no perfect chapter name for this function, so I would rather put it near to IsOperation and imho 5.5-2 fits this. Ultimately, this is a matter of taste I guess.

@fingolfin
Copy link
Member Author

Yeah, I also find the section title "Global Variables in the Library" super confusing and rather inappropriate. And there are indeed some key concepts related to these operations/filters/categories/ etc. which don't seem to be explained in that chapter (or anywhere in the reference manual that I could find now).

I'll think some more about it...

Copy link
Contributor

@PaulaHaehndel PaulaHaehndel left a comment

Choose a reason for hiding this comment

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

The code returns the following error for ShowDeclarationsOfOperation(IsOperation);

Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `Length' on 1 arguments
The 1st argument is 'fail' which might point to an earlier problem
 at /mnt/c/gap/lib/methsel2.g:249 called from
Length( locs ) at /mnt/c/gap/lib/methwhy.g:422 called from
<function "ShowDeclarationsOfOperation">( <arguments> )
 called from read-eval loop at *stdin*:2
type 'quit;' to quit to outer loop```

lib/methwhy.g Outdated Show resolved Hide resolved
@PaulaHaehndel
Copy link
Contributor

I also wouldn't expect it in the current place and more likely would search for it in 7 Debugging and Profiling Facilities.

@fingolfin
Copy link
Member Author

fingolfin commented May 21, 2019

@PaulaHaehndel yes, it breaks if you give it IsOperation. Just like it breaks when you give it a string, the integer 42, or anything else that's not an operation. So, what are you saying -- is that an implicit request for argument validation?

@fingolfin fingolfin force-pushed the mh/ShowDeclarationsOfOperation branch from 82aa61e to c008f26 Compare May 21, 2019 14:07
@codecov
Copy link

codecov bot commented May 21, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@87794e8). Click here to learn what that means.
The diff coverage is 58.82%.

@@            Coverage Diff            @@
##             master    #3459   +/-   ##
=========================================
  Coverage          ?   85.33%           
=========================================
  Files             ?      699           
  Lines             ?   346482           
  Branches          ?        0           
=========================================
  Hits              ?   295684           
  Misses            ?    50798           
  Partials          ?        0
Impacted Files Coverage Δ
lib/methwhy.g 79% <58.82%> (ø)

@fingolfin fingolfin force-pushed the mh/ShowDeclarationsOfOperation branch from c008f26 to 6a4233d Compare May 21, 2019 14:10
When debugging issues with method installations for operations (including
attributes and properties), it can be helpful to find out what all
declarations of a given operation are. This helper function does just that.
@fingolfin fingolfin force-pushed the mh/ShowDeclarationsOfOperation branch from 6a4233d to 402644d Compare May 21, 2019 14:15
@fingolfin
Copy link
Member Author

@PaulaHaehndel regarding the error with IsOperation as input: I stand corrected, IsOperation is an operation, I badly mixed this up with something else. My apologies! I've now added a check for that.

@fingolfin
Copy link
Member Author

I'll merge this now, and then provide another PR to improve the arrangement of things a bit. Thank you all for your feedback!

@fingolfin fingolfin merged commit 649dffb into gap-system:master May 21, 2019
@fingolfin fingolfin deleted the mh/ShowDeclarationsOfOperation branch May 21, 2019 21:58
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 21, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants