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

lib/string.gi: add \+ for strings, shows more helpful error #1314

Merged
merged 1 commit into from
Jun 23, 2017

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented May 8, 2017

This is an alternative to PR #1313, which instead actually concatenates its inputs.

@codecov
Copy link

codecov bot commented May 8, 2017

Codecov Report

Merging #1314 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1314      +/-   ##
==========================================
- Coverage    61.7%    61.7%   -0.01%     
==========================================
  Files         916      916              
  Lines      277105   277106       +1     
  Branches    15320    15321       +1     
==========================================
  Hits       170985   170985              
- Misses     102128   102130       +2     
+ Partials     3992     3991       -1
Impacted Files Coverage Δ
lib/string.gi 39.45% <0%> (-0.06%) ⬇️
src/hpc/threadapi.c 28.27% <0%> (-0.29%) ⬇️
hpcgap/src/vars.c 40% <0%> (+0.09%) ⬆️
src/funcs.c 63.56% <0%> (+0.27%) ⬆️

@olexandr-konovalov
Copy link
Member

At least this alternative seems uncontroversial and makes GAP more user-friendly.

@ChrisJefferson
Copy link
Contributor

Maybe add some tests to this, just for sanity, then I think we should merge it.

@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Jun 9, 2017

Indeed, adding a test as @ChrisJefferson suggests may have sense (for example, if a package will appear, which installs a method for \+ to concatenate strings). After that I suggest to merge this a solution - it does not change existing functionality, but makes GAP more user-friendly.

@olexandr-konovalov olexandr-konovalov requested review from olexandr-konovalov and removed request for olexandr-konovalov June 9, 2017 22:09
@fingolfin
Copy link
Member Author

I will not add tests to this, but anybody else is of course welcome to

@ChrisJefferson ChrisJefferson merged commit 6b705d5 into gap-system:master Jun 23, 2017
@fingolfin fingolfin deleted the mh/concat-str-error branch July 2, 2017 22:04
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.

3 participants