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

Refactor functions into submodule by GiacomoPope · Pull Request #67 · flintlib/python-flint · GitHub

  1. flintlib
  2. python-flint

Repository navigation

Refactor functions into submodule - #67

Merged
oscarbenjamin merged 12 commits into
flintlib:masterfrom
GiacomoPope:refactor-functions
Sep 7, 2023
ConversationCommits (12)ChecksFiles changed
Merged

Refactor functions into submodule#67
oscarbenjamin merged 12 commits into
flintlib:masterfrom
GiacomoPope:refactor-functions

Conversation

GiacomoPope commented Sep 6, 2023

Copy link
Copy Markdown
Contributor

This PR aims to finish the work on refactoring to remove all include, fixing issue #15.

The last file, functions.pyx has been renamed to eval_helper.pyx and is used in doctests for various flint types.

oscarbenjamin reviewed Sep 6, 2023
View reviewed changes
Comment thread src/flint/types/acb.pyx Outdated
the real sign function.

>>> from flint import showgood
>>> from flint.types.eval_helper import showgood

oscarbenjamin Sep 6, 2023

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

The docstring should show how users are expected to import something.

Does from flint import showgood not work with these changes?

GiacomoPope Sep 6, 2023

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

I could import showgood as all the other types are so it's available from flint itself but I left it this way as my understanding it was more for debugging?

The change should be minor I hope, happy for you to make the call

oscarbenjamin Sep 6, 2023

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

It is shown widely in the docs so it should be treated as an end user function. Previously the docs always showed it as being imported from flint so we should preserve that for now (and continue to show that in the docs).

I think that from flint import showgood should continue to work and should be what is shown in the docstrings.

GiacomoPope Sep 6, 2023

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

Okay, I'm happy to make the changes to fix this (tomorrow, sleep now). Won't be more than a few extra lines of code :)

GiacomoPope commented Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

Considering showgood() is a user facing function which we want to import, I have put this in

  • src/flint/functions/showgood.pyx

and then I make this available in __init__.py on the module so

from flint import showgood

works as expected. flint/functions can then be used for similar user-facing util functions.

oscarbenjamin commented Sep 7, 2023

Copy link
Copy Markdown
Collaborator

This seems to duplicate the code from functions.pyx in two places.

Also an empty .pdx file has been added (I don't know what a .pdx file is).

GiacomoPope closed this Sep 7, 2023
GiacomoPope deleted the refactor-functions branch September 7, 2023 11:00
GiacomoPope restored the refactor-functions branch September 7, 2023 11:02

GiacomoPope commented Sep 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@oscarbenjamin I did something stupid when cleaning up and deleted the wrong branch (trying to do too many things) So sorry for the mess up.

GiacomoPope reopened this Sep 7, 2023

oscarbenjamin commented Sep 7, 2023

Copy link
Copy Markdown
Collaborator

This has a merge conflict now.

GiacomoPope commented Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

Resolved, I can't keep up with all the versions!

oscarbenjamin reviewed Sep 7, 2023
View reviewed changes
Comment thread src/flint/__init__.py

oscarbenjamin commented Sep 7, 2023

Copy link
Copy Markdown
Collaborator

If this is adding a new package then it also needs to be add here:

python-flint/setup.py

Lines 66 to 73 in 24267a5

packages=[
'flint',
'flint.flintlib',
'flint.flint_base',
'flint.types',
'flint.utils',
'flint.test',
]

oscarbenjamin commented Sep 7, 2023

Copy link
Copy Markdown
Collaborator

Looks good. Thanks!

oscarbenjamin merged commit 68249c8 into flintlib:master Sep 7, 2023
oscarbenjamin mentioned this pull request Sep 16, 2023
Merged
GiacomoPope deleted the refactor-functions branch August 12, 2024 16:41
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

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL