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

Renamed DigraphDijkstra to DigraphShortestPaths #730

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

devansh2605
Copy link

@mtorpey
Copy link
Collaborator

mtorpey commented Feb 12, 2025

This is looking good! The only problem now (which I didn't mention before) is that we want to preserve backwards compatibility. So, now that DigraphDijkstra is gone, we want to introduce DigraphDijkstra as a synonym for DigraphShortestPaths. Look at how DeclareSynonym is used elsewhere in the package, and add new commits to this branch to add them to this PR.

@james-d-mitchell
Copy link
Member

Might even make sense to do something like we do is Semigroups and mark DigraphDijkstra as obsolete:

https://github.com/semigroups/Semigroups/blob/7cc3070a8fda8d9b866b5864357dff491c133eb9/gap/obsolete.gi#L11

@james-d-mitchell james-d-mitchell added the not-backwards-compatible A label for PRs that break backwards compatibility label Feb 14, 2025
@james-d-mitchell james-d-mitchell changed the title Renamed DigraphDijkstra to DigraphShortestPaths Renamed DigraphDijkstra to DigraphShortestPaths Feb 14, 2025
@mtorpey mtorpey changed the base branch from main to dev-2.0 February 19, 2025 16:51
@mtorpey mtorpey changed the base branch from dev-2.0 to main February 19, 2025 16:52
@devansh2605
Copy link
Author

devansh2605 commented Feb 26, 2025

Currently, I have renamed DigraphDijkstra to DigraphShortestPaths and also declared it as a synonym using DeclareSynonym. Please let me know if I should proceed with #735.

I had a suggestion on how to implement #735 as well:

  1. We could define a helper function DeclareDeprecatedSynonym which creates something like a wrapper that prints the warning and uses the Apply fucntion of GAP to forward the arbitrary number of arguments to the new function.
  2. We can then bind this wrapper to the name DigraphDijkstra (a string as can be seen in DeclareSynonym) using a DeclareDeprecatedSynonym. This way every call to DigraphDijkstra triggers the warning before executing DigraphShortestPaths and then can run the updated function name which is DigraphShortestPaths in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-backwards-compatible A label for PRs that break backwards compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants