FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

rename l_function method name by GiacomoPope · Pull Request #212 · flintlib/python-flint · GitHub

  1. flintlib
  2. python-flint

Repository navigation

rename l_function method name - #212

Merged
oscarbenjamin merged 3 commits into
flintlib:mainfrom
GiacomoPope:l_function_name
Sep 9, 2024
ConversationCommits (3)ChecksFiles changed
Merged

rename l_function method name#212
oscarbenjamin merged 3 commits into
flintlib:mainfrom
GiacomoPope:l_function_name

Conversation

GiacomoPope commented Sep 2, 2024

Copy link
Copy Markdown
Contributor

Fixes #210

oscarbenjamin reviewed Sep 2, 2024
View reviewed changes
Comment thread src/flint/types/dirichlet.pyx Outdated
Comment on lines +192 to +193
# For backwards compatibility we allow self.l(s) see Issue #210
l = l_function # no-cython-lint

oscarbenjamin Sep 2, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

GiacomoPope Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

oh sorry, i just missed this

oscarbenjamin commented Sep 2, 2024

Copy link
Copy Markdown
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
5af4b65 
reviewer feedback

GiacomoPope commented Sep 2, 2024

Copy link
Copy Markdown
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 changed the base branch from master to main September 2, 2024 15:42
oscarbenjamin merged commit 8b2a3ee into flintlib:main Sep 9, 2024

oscarbenjamin commented Sep 9, 2024

Copy link
Copy Markdown
Collaborator

Okay, let's get it in.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

oscarbenjamin oscarbenjamin left review comments

Assignees

No one assigned

Labels

type: code quality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

E743 ambiguous function definition

2 participants

Back | FazBrowse Home | New Git URL