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

allow repeated use of same DeclareSynonym call #835

Merged
merged 1 commit into from
Jun 25, 2016

Conversation

bh11
Copy link
Contributor

@bh11 bh11 commented Jun 21, 2016

Description of changes (for the release notes)

This allows the same synonym to be declared in different places, e.g., different packages, similar to DeclareOperation. This might be useful to remove package dependencies.

For example, this would allow an alternative resolution of the dependencies between the upcoming PrimitiveGroups package and IRREDSOL discussed in #791 (by making the same declarations in both packages).

BTW., I don't think that coll.gd is a good place for this code, should it be moved elsewhere (oper.g or variable.g)?

I don't think that this needs test code.

This allows the same synonym to be declared in different places, e.g., different packages, similar to DeclareOperation. This might be useful to remove package dependencies.
@fingolfin
Copy link
Member

I think this is a good idea, and the implementation "looks" good to me, too.

I am impartial as to whether to move the code or not. There are countless functions in GAP which to me seem to be in an "illogical" place, but often it is not quite clear to me what a better place would be, plus in the end it doesn't really matter to me where they are declared -- when looking for function declarations and implementations, I never rely on filenames anyway.

As to tests: I think no feature is so simple as not to require tests. For this changes, a test that verifies that declaring the same synonym twice works as expected seems like a good idea. However, I agree in so far as this is "low priority": There are many other things in GAP which need a test more badly than this piece of code... So while I'd appreciate such a test, I also don't think its absence is a strong reason for not merging this PR... :-)

@bh11 bh11 merged commit 3acf246 into gap-system:master Jun 25, 2016
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Jan 20, 2018
@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.

None yet

3 participants