search

oscar-system / Oscar.jl

A comprehensive open source computer algebra system for computations in algebra, geometry, and number theory.
https://www.oscar-system.org
Other
339 stars 120 forks source link

Rename `mat` to `matrix` ? #2885

Closed felix-roehrich closed 11 months ago

felix-roehrich commented 11 months ago

I believe the following two functions should be renamed to matrix. Is there any reason why they were named mat ? git blame shows @fieker and @wbhart, respectively. If there are no protests, I can refactor this.

https://github.com/oscar-system/Oscar.jl/blob/0c82301ca340ad6998a6700488f2b80a04fdbd3e/experimental/GModule/Cohomology.jl#L1617-L1619

https://github.com/Nemocas/AbstractAlgebra.jl/blob/e1c902fc1d45281d65a7cb7b4dda4bb55a3f77d4/src/ModuleHomomorphism.jl#L13

fieker commented 11 months ago

On Thu, Oct 05, 2023 at 04:55:30AM -0700, Felix Röhrich wrote:

I believe the following two functions should be renamed to matrix. Is there any reason why they were named mat ? git blame shows @fieker and @wbhart, respectively. If there are no protests, I can refactor this. It was less typing....

But please, try to avoid merge conflicts in CF-WorkInProgress, pr 2870

https://github.com/oscar-system/Oscar.jl/blob/0c82301ca340ad6998a6700488f2b80a04fdbd3e/experimental/GModule/Cohomology.jl#L1617-L1619

https://github.com/Nemocas/AbstractAlgebra.jl/blob/e1c902fc1d45281d65a7cb7b4dda4bb55a3f77d4/src/ModuleHomomorphism.jl#L13

-- Reply to this email directly or view it on GitHub: https://github.com/oscar-system/Oscar.jl/issues/2885 You are receiving this because you were mentioned.

Message ID: @.***>

felix-roehrich commented 11 months ago

It was less typing....

Understandable, but since these functions are exported, I think it is better to be careful with such abbreviations, as we don't want to pollute the global namespace. I found this because I wanted to name a variable mat in the CLI.

I will rename the occurrences and see if a rebase is possible without conflicts. If not, this can be done after #2870 is merged.

felix-roehrich commented 11 months ago

Closed with #2903