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

adjusted a statement about InfoObsolete #4654

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

ThomasBreuer
Copy link
Contributor

Variables created by DeclareObsoleteSynonym and DeclareObsoleteSynonymAttr
print a message if the InfoObsolete level is high enough.
The current default threshold is 2 for DeclareObsoleteSynonym
and 1 for DeclareObsoleteSynonymAttr,
but the documentation claimed 1 for both.

Perhaps it would be better to change the code to use default 1 (or 2?)
in both cases.

Variables created by `DeclareObsoleteSynonym` and `DeclareObsoleteSynonymAttr`
print a message if the `InfoObsolete` level is high enough.
The current default threshold is 2 for `DeclareObsoleteSynonym`
and 1 for `DeclareObsoleteSynonymAttr`,
but the documentation claimed 1 for both.

Perhaps it would be better to change the code to use default 1 (or 2?)
in both cases.
@ThomasBreuer ThomasBreuer added topic: documentation Issues and PRs related to documentation release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Sep 12, 2021
@fingolfin
Copy link
Member

I believe the use of two levels was introduced by @ChrisJefferson in PR #2923. So perhaps he'd like to chime in here. Also @alex-konovalov might.

Generally said, I am not sure whether we really want to remove obsolete stuff, ever, unless we have strong reasons for doing so (e.g.: a function requires a certain internal way to represent things in order to work; we want to change those internals, but don't want to put efforts into rewriting an obsolete function for use with the new setup). After all, if there is no immediate gain, why inconvenience our users? We also may want to warn about some obsoletes (without removing them) if their user suggests potentially inefficient or unsafe code, though.

@ThomasBreuer
Copy link
Contributor Author

O.k., this topic is not relevant enough for more comments. The solution is perhaps not optimal, but it makes the world a little bit better.

@ThomasBreuer ThomasBreuer merged commit 2cf8036 into gap-system:master Sep 27, 2021
@ThomasBreuer ThomasBreuer deleted the TB_InfoObsolete branch September 27, 2021 08:19
@ChrisJefferson
Copy link
Contributor

Just to say, I am happy this was merged, and believe it fixed an earlier mistake of mine. I missed the PR in the pile of emails I received while on holiday. Sorry.

@olexandr-konovalov
Copy link
Member

Perhaps it would be better to change the code to use default 1 (or 2?) in both cases.

I think so (if yes, then to 1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: documentation Issues and PRs related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants