fix: accept both 'impl' and 'implementation' parameters for GF()

[adnanbhat7]

GF: Accept both impl and implementation parameters

Fixes #40806

📝 Description

This PR fixes an inconsistency in the GF() constructor where GF(25, impl='givaro') was accepted but GF(25, implementation='givaro') raised an error or was silently ignored.

The Problem

The GF() function internally used the parameter name impl but users naturally expected the more descriptive implementation to work. When implementation was used, it was either ignored or caused an error, leading to confusion.

The Solution

• Modified create_key_and_extra_args() to accept both impl and implementation parameters
• Added logic to map implementation to the internal impl variable
• Added clear error messaging when both parameters are specified
• Maintained full backward compatibility with existing code

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation and checked the documentation preview.

[vincentmacri]

Looks good at first glance. I'll take a more careful look after the CI runs.

@sagemath/core can someone approve the CI run?

[github-actions]

Documentation preview for this PR (built with commit 8b654d0; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[vincentmacri]

Thank you for your contribution!

CI looks good other than the linter error, make sure you fix that. This approach looks good, but I'd like you to clean it up just a little bit more. Specifically, #40806 says that we should prefer implementation and eventually deprecate impl. Deprecating impl is out of scope for this PR and requires further discussion. But we should definitely prefer implementation over impl, and the documentation and function signatures should reflect that.

To that effect, can you make these changes:

1. Change the impl keyword argument in the relevant methods to implementation, and get impl from **kwds instead of the other way around.
2. Reword the documentation to make it clear that implementation is preferred, but that impl is supported for backwards compatibility.
3. Change any examples in the docstrings that use impl to use implementation. I don't expect you to comb through our entire codebase for this, but do it for the docstrings of any methods you're modifying anyway.
4. As mentioned above, fix the lint error. Since you're a first-time contributor GitHub permissions make running the CI kind of annoying. To speed things up, please run at least the ruff linter locally and make sure our requires rules are passing on any files you've changed before pushing your changes (see https://doc.sagemath.org/html/en/developer/tools.html for how to run ruff locally)
5. (Optional) For any methods where you change the method signature, add type annotations where possible. We're slowly working on adding more type annotations to Sage, and if you're modifying a line might as well add type annotations while you're at it. This should be fairly easy for bool and str arguments, integer arguments are weird because Sage has two integer types so don't worry about those for now.

Also might as well merge the latest develop while you're at it.

[vincentmacri]

Tagging @user202729 who opened #40806 in case they have anything else to add.

[adnanbhat7]

@vincentmacri
Ready for review
changes
1.Changed impl to implementation in the method signature and updated logic to extract impl from **kwds for backwards compatibility
2.Updated documentation to prefer implementation over impl, noting backwards compatibility
3.Changed all examples in docstrings from impl to implementation
4.Fixed the lint error on line 678 by changing map(Integer, order) to [Integer(x) for x in order]
5.Added type annotations: implementation: str | None, check_prime: bool, check_irreducible: bool, prefix: str | None, repr: str | None, elem_cache: bool | None
6.Ran ruff-minimal locally and verified it passes

[nbruin]

Can you check the merge history of the branch? github reports 164 commits on this branch, with many changes that are unrelated to what the ticket is about.

One commit showing is, for instance: Updated SageMath version to 10.8.beta9.

in sagemath:develop there is a corresponding commit (with the same hash!):

5c8d9e9

Normally such commits should not be reported, since the comparison is made against sagemath:develop. I suspect updates were merged in the wrong way. It might be good to go back to a cleaner point in your branch and redo the merge of develop into it. You'd then have to force-push the resulting branch to this pull-request. Hopefully you'll get a cleaner history that way.

I'm only noticing the problem and don't have much experience with fixing this kind of thing. Advice from a git expert most welcome.

[adnanbhat7]

@nbruin

sorry i am new to open source , please review this now i think i did another mistake while merging .

[dimpase]

your branch should be based on the current beta.
You can take a diff w.r.t. it, create a new branch
containing a single commit with this diff, and force-push this to this PR.

Assuming you are on the current PR branch:

git fetch origin develop
git diff origin/develop > /tmp/mypr
git checkout -b cleanpr origin/develop
git apply /tmp/mypr
git add ....
git commit
git push ....

[adnanbhat7]

@dimpase
thank you helping out , i actually did something similar and was able to create a new cleaner pr actually, i was trying to keep this pr intact and clean it , but was unable to do so , still don't know whether that was possible or not.
well here is the new pr you can review it #41189