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

When printing function bodies, avoid some redundant spaces #1498

Merged
merged 1 commit into from
Sep 15, 2017

Conversation

fingolfin
Copy link
Member

In particular, don't print 'if x then', but rather 'if x then' (note the extra space in the former), and similar after "local", and before "do".

Resolves #473

This PR will fail lots of tests, because so many of them contain printed code, which now of course is "formatted wrong". It shouldn't be too hard to fix all that with some calls to sed or perl, but I don't have the time to do that now, and perhaps somebody wants to step up and do it?

@fingolfin fingolfin added the good first issue Issues that can be understood and addressed by newcomers to GAP development label Jul 12, 2017
@ChrisJefferson
Copy link
Contributor

I like this. My only comment is if there are other changes we want to make at the same time.

@fingolfin
Copy link
Member Author

@ChrisJefferson I am not sure whether your comment is a question, or a statement (in the latter case, could you say what you have in mind?)

@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Aug 21, 2017
@olexandr-konovalov olexandr-konovalov self-assigned this Aug 21, 2017
@olexandr-konovalov
Copy link
Member

This would be good to do. I should be able to push to this branch - may try.

@frankluebeck
Copy link
Member

This PR will fail lots of tests, because so many of them contain printed code, which now of course is "formatted wrong". It shouldn't be too hard to fix all that with some calls to sed or perl, but I don't have the time to do that now, and perhaps somebody wants to step up and do it?

Are you aware that GAP has tools to change test files and manuals?

For test files: see ?Reference: Test, in particular the entry .rewriteToFile of the options record.

For manual examples: see GAPDoc: RunExamples, in particular the entry .changeSources in the options record (this even works if the manual is collected from many program files).

@frankluebeck
Copy link
Member

Concerning the content of this pull request: All changes look sensible to me.

@fingolfin
Copy link
Member Author

@frankluebeck Yes, I am aware, I just couldn't be bothered to figure out how to pass those options to TestDirectory correctly; and then there are the manual examples, too, which probably need some manual work (but hopefully only a very few are affected).

So this is not a difficult task, just one that takes time (which I rather want to spend on other things right now)

@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Sep 12, 2017

@fingolfin looking at

# But found:
function (  )
    local a, b, c;
    if IsBound( a) then
        Print( "1" );
    fi;
 ...

Is it also possible to add a padding after a in the line if IsBound( a) then ?

@codecov
Copy link

codecov bot commented Sep 13, 2017

Codecov Report

Merging #1498 into master will increase coverage by 0.09%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #1498      +/-   ##
==========================================
+ Coverage   64.44%   64.54%   +0.09%     
==========================================
  Files        1002     1002              
  Lines      326861   328423    +1562     
  Branches    13216    13485     +269     
==========================================
+ Hits       210661   211990    +1329     
- Misses     113334   113530     +196     
- Partials     2866     2903      +37
Impacted Files Coverage Δ
src/c_methsel1.c 67.61% <ø> (-0.03%) ⬇️
hpcgap/src/c_type1.c 70.17% <ø> (ø) ⬆️
src/c_filter1.c 95.85% <ø> (ø) ⬆️
src/c_type1.c 72.59% <ø> (ø) ⬆️
hpcgap/src/c_filter1.c 96.44% <ø> (ø) ⬆️
hpcgap/src/c_oper1.c 86.4% <ø> (ø) ⬆️
hpcgap/src/c_methsel1.c 67.39% <ø> (ø) ⬆️
src/c_oper1.c 87.42% <ø> (-0.05%) ⬇️
src/stats.c 73.07% <100%> (ø) ⬆️
src/calls.c 52.82% <100%> (ø) ⬆️
... and 18 more

@olexandr-konovalov
Copy link
Member

@fingolfin rebased and pushed with tests updated and extended. Waiting for CI results.

@@ -38,4 +38,14 @@ f := ({} -> IsBound(BADVARNAME[BADLISTNAME]) );;
^
gap> f();
Error, Variable: 'BADVARNAME' must have an assigned value

# Printing IsBound statements
gap> Print(function(l,n) return IsBound(l[n]);end);
Copy link
Member Author

Choose a reason for hiding this comment

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

Use Display instead of Print to make the tests pass (I think).

@olexandr-konovalov
Copy link
Member

@fingolfin thanks - pushed the fix to see the effect, will edit the branch later while rebasing

@fingolfin
Copy link
Member Author

That push seems to have helped.

@olexandr-konovalov
Copy link
Member

@fingolfin rebased and squashed into one commit - waiting for CI results.

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Sep 14, 2017
@fingolfin
Copy link
Member Author

@alex-konovalov looks good to me, but I can't approve this, as it's my PR ;-). Also, you really did by far the most work, and that commit is now authored by me. Perhaps change the commit author to yourself, and then merge? (I don't mind if it is merged as is, just want to make clear that I think Alex did by far most of it).

Also: should this be mentioned in the released notes? Personally, I consider it not important enough, but it is user visible, so it is a border case...

@olexandr-konovalov
Copy link
Member

@fingolfin thanks, but idea was yours and the initial study and modification of the code too,
so I am sure it should be yours. I can amend the commit message and mention there parts that I have added (it was not practical to keep them in a separate commit, since they would not be atomic otherwise).

I'd mention this in release notes, exactly by the mentioned reason.

In particular, don't print 'if x  then', but rather 'if x then'
(note the extra space in the former), and similar after "local",
and before "do". This resolves gap-system#473

Also, use consistent padding while printing IsBound statements,
update test files and add new test for printing IsBound calls,
as implemented by @alex-konovalov
@ChrisJefferson ChrisJefferson merged commit b8a6c45 into gap-system:master Sep 15, 2017
@fingolfin fingolfin deleted the mh/tweak-code-printing branch September 18, 2017 12:36
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that can be understood and addressed by newcomers to GAP development 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.

Why does GAP put two spaces after local and before then when printing functions?
4 participants