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

Replace RecFields by RecNames, mark it as obsolete #1331

Merged
merged 1 commit into from
Jun 27, 2017

Conversation

fingolfin
Copy link
Member

This function name was used in GAP 3 but has been deprecated in GAP 4.

@olexandr-konovalov
Copy link
Member

Obviously +1, good catch! Surprised that it was not in obsoletes already.

@codecov
Copy link

codecov bot commented May 10, 2017

Codecov Report

Merging #1331 into master will increase coverage by <.01%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #1331      +/-   ##
==========================================
+ Coverage   63.26%   63.26%   +<.01%     
==========================================
  Files        1011     1011              
  Lines      341389   341389              
  Branches    14177    14177              
==========================================
+ Hits       215980   215982       +2     
  Misses     122227   122227              
+ Partials     3182     3180       -2
Impacted Files Coverage Δ
lib/record.g 72.22% <ø> (ø) ⬆️
lib/obsolete.gd 28.35% <ø> (ø) ⬆️
lib/string.gi 39.51% <0%> (ø) ⬆️
lib/test.gi 45.09% <100%> (ø) ⬆️
lib/sgpres.gi 73.04% <100%> (ø) ⬆️
lib/memusage.gi 86.08% <100%> (ø) ⬆️
src/listfunc.c 76.77% <0%> (-0.18%) ⬇️
src/funcs.c 62.97% <0%> (ø) ⬆️
src/hpc/traverse.c 80.62% <0%> (+0.38%) ⬆️
src/objset.c 41.59% <0%> (+0.56%) ⬆️

@frankluebeck
Copy link
Member

@fingolfin wrote:

This function name was used in GAP 3 but has been deprecated in GAP 4.

I do not agree that RecFields is deprecated. In 2000 there was a discussion about name changes between GAP 3 and GAP 4. Within that discussion I complained that the introduction of RecNames (which probably should have been RecordNames) is a unnecessary incompatibility with GAP 3. RecFields was then reintroduced to the library as synonym to RecNames.
Of course, it is a documentation bug that this is not mentioned in the documentation; this would be corrected by the alternative pull request "Document RecFields" (#1335).

Deprecating RecFields would essentially break all GAP 4 code I have ever written. It is also used in packages like GAPDoc or CTblLib. Note that in the latter case many files are maintained such that they can be used with GAP 3 and GAP 4, this would be broken by this pull request.

Also, some CHEVIE users who only occasionally use GAP 4, would be annoyed, if RecFields stopped to work.

In my opinion it doesn't hurt to keep this synonym.

@fingolfin
Copy link
Member Author

This PR does not remove the synonym, so nothing is broken by it.

This function name was used in GAP 3 but has been deprecated in GAP 4.
@james-d-mitchell
Copy link
Contributor

I'm in favour of merging this PR too, duplicate names for things like this is just confusing.

@ChrisJefferson
Copy link
Contributor

I think we should merge this. It might have been better if we'd stuck with RecFields instead of introduce RecNames, but now RecNames is clearly the "standard" name. The name will still work for older code of course.

@frankluebeck
Copy link
Member

It seems that it is mainly me who has always used RecFields. So, I agree to merge this pull request.

But it would be good if RecFields remained working as it did the last 20 years for quite some time because I've used RecFields in all GAP4 code I ever wrote.

@ChrisJefferson
Copy link
Contributor

My preference would be while we might discourage people from using alternative names, we should never brake them unless we made such huge changes to GAP we broke many programs anyway. I don't think anyone really wants to do that.

@fingolfin fingolfin merged commit 7310982 into gap-system:master Jun 27, 2017
@fingolfin fingolfin deleted the mh/RecFields branch July 2, 2017 22:04
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants