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 BindingsOfClosure helper #3606

Merged
merged 1 commit into from
Aug 30, 2019

Conversation

fingolfin
Copy link
Member

Partially addresses #3569.

Well, or maybe it addresses it completely, though one still might want to enhance the output of Display(func) if there are bindings? Somebody else could do that based on this PR, though.

I also guess it would be nice to have documentation for BindingsOfClosure...

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Aug 19, 2019
@codecov
Copy link

codecov bot commented Aug 19, 2019

Codecov Report

Merging #3606 into master will decrease coverage by 0.9%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #3606      +/-   ##
==========================================
- Coverage   84.55%   83.65%   -0.91%     
==========================================
  Files         697      697              
  Lines      345018   345135     +117     
==========================================
- Hits       291723   288709    -3014     
- Misses      53295    56426    +3131
Impacted Files Coverage Δ
lib/function.gi 86.45% <100%> (+2.5%) ⬆️
src/vars.c 93.85% <71.42%> (-0.21%) ⬇️
lib/grpreps.gi 37.36% <0%> (-46.71%) ⬇️
lib/ffeconway.gi 68.82% <0%> (-17.68%) ⬇️
lib/grpramat.gi 71.21% <0%> (-16.61%) ⬇️
lib/gprd.gi 59.15% <0%> (-15.44%) ⬇️
lib/polyconw.gi 65.24% <0%> (-14.19%) ⬇️
lib/grplatt.gi 64.4% <0%> (-11.83%) ⬇️
lib/helpview.gi 29.05% <0%> (-9.69%) ⬇️
lib/clashom.gi 71.17% <0%> (-9.24%) ⬇️
... and 219 more

Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

@DominikBernhardt and I have reviewed this together. Everything looks fine, and it's nice to have. Although we agree with you that it would be nice to have some documentation that gives the purpose and expected output of BindingsOfClosure.

@DominikBernhardt DominikBernhardt added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes gapsingular2019 Issues and PRs that arose at https://opendreamkit.org/meetings/2019-04-02-GAPSingularMeeting labels Aug 20, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 84.323% when pulling c2d65b2 on fingolfin:mh/BindingsOfClosure into 0f7a8d6 on gap-system:master.

@wilfwilson
Copy link
Member

@fingolfin Do you intend to document BindingsOfClosure in this PR?

@fingolfin
Copy link
Member Author

I don't have the time and energy to work on documentation for BindingsOfClosure right now. I am not sure we'd want to document it as such globally, either... so there are multiple paths forward:

  1. We merge this now, then for a few people BindingsOfClosure already is useful; and update Add a convenient way to display bindings inside of closures #3569 to mention it, suggesting that somebody else submit a PR to enhance Display for closures using it (and also that they could consider documenting BindingsOfClosure as well).
  2. We merge it until somebody has time and energy to write some documentation, and tack it onto this PR.

I think 1. is better because it means this code won't bitrot, and people can benefit from it already (even if the number of people doing so would be small, as it is not documented). But either is fine by me.

@wilfwilson
Copy link
Member

I'm happy for us to do 1. I thought that perhaps your sentence

I also guess it would be nice to have documentation for BindingsOfClosure...

suggested that you were maybe thinking of creating documentation, which is why I asked about it.

@wilfwilson wilfwilson merged commit 3ac8fb0 into gap-system:master Aug 30, 2019
@fingolfin fingolfin deleted the mh/BindingsOfClosure branch August 30, 2019 17:16
@dimpase dimpase 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 Feb 4, 2020
@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
gapsingular2019 Issues and PRs that arose at https://opendreamkit.org/meetings/2019-04-02-GAPSingularMeeting kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants