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

rename l_function method name #212

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

GiacomoPope
Copy link
Contributor

Fixes #210

Comment on lines 192 to 193
# For backwards compatibility we allow self.l(s) see Issue #210
l = l_function # no-cython-lint
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't any point in doing this unless the docstring is updated to suggest calling the new function name.

I would do it like:

def l_function(self, s):
    """
    >>> l_function()
    123
    """
    ...


def l(self, s):
    """
    Alias for :meth:`l_function`
    """
    return self.l_function(s)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry, i just missed this

@oscarbenjamin
Copy link
Collaborator

I'm not sure it is worth changing the name here. I don't know much about this function and I don't know what it would normally be called. If the linter wasn't complaining then it would not have been suggested to change this so I would probably just leave it I think.

Putting "function" in the name seems sort of redundant when most methods are functions.

reviewer feedback
@GiacomoPope
Copy link
Contributor Author

I think l_function makes sense, as it's a mathematical object known as a l-function (https://en.wikipedia.org/wiki/Dirichlet_L-function), obviously all the code is built with methods/functions but just calling it l doesnt seem very helpful to me. Happy to just close this though if you disagree

@oscarbenjamin oscarbenjamin changed the base branch from master to main September 2, 2024 15:42
@oscarbenjamin oscarbenjamin merged commit 8b2a3ee into flintlib:main Sep 9, 2024
40 checks passed
@oscarbenjamin
Copy link
Collaborator

Okay, let's get it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E743 ambiguous function definition
2 participants