Various changes related to obsolete symbols #1452
Conversation
Instead, just say they might be removed in "a future release". Which we might or might not do...
Previously we used CALL_WITH_CATCH, which is (a) undocumented, and (b) does additional things we don't actually want here.
Codecov Report
@@ Coverage Diff @@
## master #1452 +/- ##
==========================================
- Coverage 63.79% 61.9% -1.89%
==========================================
Files 1009 533 -476
Lines 337592 84317 -253275
Branches 13483 13453 -30
==========================================
- Hits 215375 52200 -163175
+ Misses 119253 29167 -90086
+ Partials 2964 2950 -14
|
| detect that an obsolete variable is used at runtime (this detection is | ||
| possible, however, only for obsolete variables declared using | ||
| <C>DeclareObsoleteSynonym</C>). | ||
| detect that an obsolete variable is used at runtime. |
There was a problem hiding this comment.
Curious why the limitation of using DeclareObsoleteSynonym may be dropped.
There was a problem hiding this comment.
There is no limitation here being dropped. Rather, what happens is this: Some official documentation references to an unofficial and undocumented internal function DeclareObsoleteSynonym, which it should not do. I then tried to rephrase the parens to not use it, but that made it realize that it actually does not convey much if any useful information to the user; in summary, the only thing the user might learn is that sometimes, we might fail to detect use of an obsolete, for reasons the user has no chance to understand ("so what is this DeclareObsoleteSynonym? And why don't they always use it?).
However, re-reading it again now, I realize another issue with this paragraph: It refers to "variables", which is highly misleading. Sure, from a purely technical point of view, it is correct, because things like Group and IsGroup are strictly speaking variables which contain or reference functions, filters, etc.
But a user may very well read this as "global variable". Which is ironic, because that's the only kind of obsolete object we currently cannot warn about it being used.
So, I think this paragraph should instead refer to "functions" (or perhaps "functions, operation, properties, attributes, filters, ..." ?!).
There was a problem hiding this comment.
Or perhaps change it to obsolete identifier, then explain that we cannot resp. "do not warn about obsolete identifiers for global variables, only for global functions, operations, attributes and properties".
There was a problem hiding this comment.
I had no chance to comment back on this, and now the PR has been merged without further discussions. Perhaps documenting DeclareObsoleteSynonym and leaving the text as it was would also be an option.
| @@ -76,9 +76,9 @@ | |||
| ## <#/GAPDoc> | |||
| ## | |||
|
|
|||
| BIND_GLOBAL( "DeclareObsoleteSynonym", function( name_obsolete, name_current, desc ) | |||
| BIND_GLOBAL( "DeclareObsoleteSynonym", function( name_obsolete, name_current ) | |||
There was a problem hiding this comment.
So, I understand this as that you're suggesting not to specify the version in which it may be deleted, just because it's not guaranteed to happen.
There was a problem hiding this comment.
It's not just |not guaranteed", it is completely unclear whether it'll ever happen. If we leave a value in, somebody has to take care of updating it before it becomes obsolete -- which clearly here did not happen.
And as long as these are used by lots of packages, realistically we are not going to make them go away.
If we ever have a concrete plan for phasing out obsolete identifiers, we can still re-introduce warnings referencing specific versions -- if that'll be necessary by that hypothetical plan.
| @@ -879,18 +879,6 @@ end ); | |||
|
|
|||
| ############################################################################# | |||
| ## | |||
| #F USER_HOME_EXPAND | |||
There was a problem hiding this comment.
Just to check, are you sure it's not used anywhere?
There was a problem hiding this comment.
USER_HOME_EXPAND is not being removed, just switched to using DeclareObsoleteSynonym.
No description provided.