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

NF: Add keyword_only decorator to enforce keyword-only arguments #888

Merged
merged 11 commits into from
Jul 10, 2024

Conversation

WassCodeur
Copy link
Member

Description:

Added @keyword_only decorator to enforce keyword-only arguments in project functions.

Modification details:

  • Creation of the @keyword_only decorator to ensure that functions can only be - called with arguments specified solely by keyword.
  • Documentation of the new decorator, including usage examples and explanations of how it works.
  • Added unit tests to ensure proper operation of the new decorator.

This contribution represents the first step towards applying the @keyword_only decorator to the appropriate functions in the project. Current modifications include the creation of the decorator, associated documentation and unit tests. The next step will be to identify and apply the decorator to relevant functions throughout the project.

@WassCodeur
Copy link
Member Author

Hi @skoudoro!
I'm trying to understand why the test fails on github.

@WassCodeur WassCodeur closed this May 21, 2024
@WassCodeur WassCodeur deleted the keyword_only branch May 21, 2024 17:49
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @WassCodeur,

Thank you for this first version. See below some comments


def test_keyword_only():
@keyword_only
def f(*, a, b):
Copy link
Contributor

Choose a reason for hiding this comment

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

here, the function should be :

def f(a, b, *, c=1, d=2):
    return a+b+c+d

Comment on lines 112 to 123
raise TypeError(
(
"{}() takes 0 positional arguments but {} were given\n"
"Usage: {}({})\n"
"Please Provide keyword-only arguments: {}"
).format(
func.__name__,
len(args),
func.__name__,
params_sample_str,
params_sample_str,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not want to raise TypeError. maximum a warning

)
else:
if unexpected_params:
raise TypeError(
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not want to raise TypeError. maximum a warning

)

elif missing_params:
raise TypeError(
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not want to raise TypeError. maximum a warning

return a + b

npt.assert_equal(f(a=1, b=2), 3)
npt.assert_raises(TypeError, f, a=1, b=2, c=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

this case should not raise typeError or warnings, this is a good case


npt.assert_equal(f(a=1, b=2), 3)
npt.assert_raises(TypeError, f, a=1, b=2, c=2)
npt.assert_raises(TypeError, f, 1, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should generate a warning and not a typeError

npt.assert_equal(f(a=1, b=2), 3)
npt.assert_raises(TypeError, f, a=1, b=2, c=2)
npt.assert_raises(TypeError, f, 1, 2)
npt.assert_raises(TypeError, f, 1, b=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should generate a warning and not a typeError

npt.assert_raises(TypeError, f, a=1, b=2, c=2)
npt.assert_raises(TypeError, f, 1, 2)
npt.assert_raises(TypeError, f, 1, b=2)
npt.assert_raises(TypeError, f, a=1, b=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

this case is good and should generate nothing

npt.assert_raises(TypeError, f, 1, 2)
npt.assert_raises(TypeError, f, 1, b=2)
npt.assert_raises(TypeError, f, a=1, b=2)
npt.assert_raises(
Copy link
Contributor

Choose a reason for hiding this comment

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

this case should not exist

@skoudoro
Copy link
Contributor

Also, You do not need to close and open a new PR. a PR can be updated. When, it is open, it is better to update it.

it is very confusing to open and close multiple time the same PR. Let me know if you need more explanation

@skoudoro
Copy link
Contributor

So please, reopen this one and try to update it

@WassCodeur WassCodeur restored the keyword_only branch May 21, 2024 18:11
@WassCodeur
Copy link
Member Author

Okay, I've already reopened it. Thank you

Copy link

@itellaetxe itellaetxe left a comment

Choose a reason for hiding this comment

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

I think the functionality is correct,
Docstrings are okay, examples are given, and the test cases are sufficient.

In the CI, it looks like there is a problem when building for macos-latest for the Python versions 3.8 (ERROR: No matching distribution found for vtk>=9.1.0) and 3.10 (Error: Codecov token not found. Please provide Codecov token with -t flag. maybe something related to the workflow .yaml configuration file).
For ubuntu-latest also in Python 3.8, the test test_elements.py::test_ui_line_slider_2d_horizontal_top is not passing.

fury/decorators.py Outdated Show resolved Hide resolved
def f(a, b, *, c, d=4, e=5):
return a + b + c + d + e

npt.assert_equal(f(1, 2, c=3, d=4, e=5), 15)

Choose a reason for hiding this comment

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

I think tests are now correct.

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

I need to look deeper at this review, but for now, can you fix all warnings and rebase this PR. Thank you @WassCodeur

@itellaetxe
Copy link

Your decorator is raising the Warnings correctly, where the keyword-only functions are not being called only with keywords. You can adjust the function calls as the warning suggests to do, so you get rid of the warnings. Similar to what @deka27 did in his decorator PR.

@WassCodeur
Copy link
Member Author

Hi @skoudoro, @itellaetxe !

Thank you for your comments. I'm working on it.

@skoudoro
Copy link
Contributor

skoudoro commented Jun 4, 2024

Hi @WassCodeur,

What is the status of this PR? Can you also rebase this PR to apply the last changes.

It would be great to have all this update as soon as possible

@WassCodeur
Copy link
Member Author

Hi @skoudoro !

Okay. I'm making the last improvements. I'll push it today.

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @WassCodeur,

Can you rebase your PR, I see old work on this branch and we can not see the difference between your work and previous work.

Thank you

@@ -43,3 +46,95 @@ def doctest_skip_parser(func):
new_lines.append(code)
func.__doc__ = "\n".join(new_lines)
return func


def keyword_only(func):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename warn_on_args_to_kwargs

also, this function needs parameter like from_version and until_version

Copy link
Member Author

Choose a reason for hiding this comment

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

@skoudoro

All right, thanks for your feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

reminder here

author WassCodeur <[email protected]> 1716302141 +0000
committer WassCodeur <[email protected]> 1717549770 +0000

NF: Add keyword_only decorator to enforce keyword-only arguments

RF: Refactor keyword-only  code and Add keyword-only arguments to functions

TEST: Add test for keyword_only" decorator

Refactor code to use keyword-only arguments in several functions

NF: remove some doctest and add a short comment on line 111 to explain what the loop does in fury/decorators.py

RF: Refactor keyword-only  code and Add keyword-only arguments to functions

Refactor code to use keyword-only arguments in several functions
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @WassCodeur,

reminder to change the decorator name.

Also, this warnings is still here and this is the last one it seems:

fury/animation/tests/test_interpolators.py::test_color_interpolators
  /home/runner/work/fury/fury/fury/animation/interpolator.py:310: UserWarning: Here's how to call the Function rgb2lab: rgb2lab(rgb_value, illuminant='value', observer='value')
    return color_interpolator(keyframes, rgb2lab, lab2rgb)

Please, check the failing CI to understand what is going on. Some are normal errors, some are not.

Thank you for the future update

@@ -43,3 +46,95 @@ def doctest_skip_parser(func):
new_lines.append(code)
func.__doc__ = "\n".join(new_lines)
return func


def keyword_only(func):
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder here

@skoudoro
Copy link
Contributor

What is the status of this PR, we need to move forward @WassCodeur.

- Renamed `keyword-only` decorator to `warn_on_args_to_kwargs` for greater clarity.
- Updated `warn_on_args_to_kwargs` to include version parameters `from_version` and `until_version`.
- Added logic to raise a RuntimeError if the current version of FURY_VERSION is greater than `until_version`.
- Moved `__version__` definition to a new `fury/version.py` module to avoid circular import problems.
- Updated all functions using the decorator to reflect the new name and parameters.
- Adjusted import declarations and ensured compatibility across the code base.
Copy link

@itellaetxe itellaetxe left a comment

Choose a reason for hiding this comment

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

Extensive job modifying both the function definitions and function calls to match the requirements of your decorator. Just a small comment on a commented test function.

Maybe you could comment the test cases of the decorator a bit so it is clearer?
On the other hand, is something like deprecates_from_version being implemented? I might have overlooked it, but I could not see it.

All in all, good. GJ Wachiou

npt.assert_raises(TypeError, func, 1)


# def test_warn_on_args_to_kwargs_with_versions():

Choose a reason for hiding this comment

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

Will this function stay commented? or is it WIP?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it's WIP

Copy link
Contributor

@deka27 deka27 Jun 12, 2024

Choose a reason for hiding this comment

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

Will u check different test states for different versions as well?

Copy link
Member Author

@WassCodeur WassCodeur Jun 13, 2024

Choose a reason for hiding this comment

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

No, it'll be one test for every version.

fury/decorators.py Outdated Show resolved Hide resolved
@WassCodeur
Copy link
Member Author

Extensive job modifying both the function definitions and function calls to match the requirements of your decorator. Just a small comment on a commented test function.

Maybe you could comment the test cases of the decorator a bit so it is clearer?
On the other hand, is something like deprecates_from_version being implemented? I might have overlooked it, but I could not see it.

All in all, good. GJ Wachiou

Thank you @itellaetxe for your constructive feedback!

Yes, you're right, I have to comment on it to make it clearer. For deprecates_from_version I managed it from L151 to L161 in the decorators.py file.

…gs arguments to functions in window.py and actor.py
Copy link
Member

@robinroy03 robinroy03 left a comment

Choose a reason for hiding this comment

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

Amazing work Wachiou :)

Please see the below suggestions.

fury/decorators.py Outdated Show resolved Hide resolved
@@ -49,3 +49,29 @@ def f():
del HAVE_AMODULE
f.__doc__ = docstring
npt.assert_raises(NameError, doctest_skip_parser, f)


def test_warn_on_args_to_kwargs():
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if tests to check from_version and until_version are trivial. It'll be good if they are possible.

I think we could do something like fury.__version__ to simulate the situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @robinroy03 thanks for your comment, this pull request for the warn_on_args_to_kwargs decorator. I will focus on this with another dedicated pull request.

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @WassCodeur,

See below additional comments. Please, look at @deka27 to get some inspiration concerning the versioning.

Looking forward for the update

fury/decorators.py Outdated Show resolved Hide resolved
fury/decorators.py Outdated Show resolved Hide resolved
fury/decorators.py Outdated Show resolved Hide resolved
fury/decorators.py Outdated Show resolved Hide resolved
fury/decorators.py Outdated Show resolved Hide resolved
fury/decorators.py Outdated Show resolved Hide resolved
fury/decorators.py Outdated Show resolved Hide resolved
fury/decorators.py Outdated Show resolved Hide resolved
from fury import __version__ as FURY_VERSION

if version.parse(FURY_VERSION) > version.parse(until_version):
raise TypeError(e) from e
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the other cases ? Look and review @deka27 PR.

if version.parse(FURY_VERSION) < version.parse(from_version), no warnings no errors

@@ -32,7 +33,8 @@


class glTF:
def __init__(self, filename, apply_normals=False):
@warn_on_args_to_kwargs()
Copy link
Contributor

Choose a reason for hiding this comment

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

parenthesis not needed everywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

if I don't put a parenthesis it raises a TypeError.

fury/decorators.py Outdated Show resolved Hide resolved
@WassCodeur
Copy link
Member Author

Hi @skoudoro

Thank you for your comments, I will take into account all your remarks and fix them too.

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Great Job! thank you for this. merging

@skoudoro skoudoro merged commit 56a6177 into fury-gl:master Jul 10, 2024
17 of 29 checks passed
WassCodeur added a commit to WassCodeur/fury that referenced this pull request Jul 11, 2024
…y-gl#888)

* parent 4baa99d
author WassCodeur <[email protected]> 1716302141 +0000
committer WassCodeur <[email protected]> 1717549770 +0000

NF: Add keyword_only decorator to enforce keyword-only arguments

RF: Refactor keyword-only  code and Add keyword-only arguments to functions

TEST: Add test for keyword_only" decorator

Refactor code to use keyword-only arguments in several functions

NF: remove some doctest and add a short comment on line 111 to explain what the loop does in fury/decorators.py

RF: Refactor keyword-only  code and Add keyword-only arguments to functions

Refactor code to use keyword-only arguments in several functions

* NF: Add keyword_only decorator to enforce keyword-only arguments

* NF: Add keyword_only decorator to enforce keyword-only arguments

* RF: Refactor keyword-only  code and Add keyword-only arguments to functions

* TEST: Add test for keyword_only" decorator

* Refactor interactor.py and molecular.py

* RF: Rename and update decorator for version checks

- Renamed `keyword-only` decorator to `warn_on_args_to_kwargs` for greater clarity.
- Updated `warn_on_args_to_kwargs` to include version parameters `from_version` and `until_version`.
- Added logic to raise a RuntimeError if the current version of FURY_VERSION is greater than `until_version`.
- Moved `__version__` definition to a new `fury/version.py` module to avoid circular import problems.
- Updated all functions using the decorator to reflect the new name and parameters.
- Adjusted import declarations and ensured compatibility across the code base.

* RF: Refactor the code by removing version constraints in the call to the warn_on_args_to_kwargs decorator

* RF: Refactor warn_on_args_to_kwargs  code and Add warn_on_args_to_kwargs arguments to functions in window.py and actor.py

* RF: update the version checks in the decorator: warn_on_args_to_kwargs and update the unittest

* RF: warn_on_args_to_kwargs decorator

parent 4baa99d
author WassCodeur <[email protected]> 1716302141 +0000
committer WassCodeur <[email protected]> 1717549770 +0000

NF: Add keyword_only decorator to enforce keyword-only arguments

RF: Refactor keyword-only  code and Add keyword-only arguments to functions

TEST: Add test for keyword_only" decorator

Refactor code to use keyword-only arguments in several functions

NF: remove some doctest and add a short comment on line 111 to explain what the loop does in fury/decorators.py

RF: Refactor keyword-only  code and Add keyword-only arguments to functions

Refactor code to use keyword-only arguments in several functions

NF: Add keyword_only decorator to enforce keyword-only arguments

RF: Refactor keyword-only  code and Add keyword-only arguments to functions

Refactor interactor.py and molecular.py

RF: Rename and update decorator for version checks

- Renamed `keyword-only` decorator to `warn_on_args_to_kwargs` for greater clarity.
- Updated `warn_on_args_to_kwargs` to include version parameters `from_version` and `until_version`.
- Added logic to raise a RuntimeError if the current version of FURY_VERSION is greater than `until_version`.
- Moved `__version__` definition to a new `fury/version.py` module to avoid circular import problems.
- Updated all functions using the decorator to reflect the new name and parameters.
- Adjusted import declarations and ensured compatibility across the code base.

RF: update the version checks in the decorator: warn_on_args_to_kwargs and update the unittest

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

Successfully merging this pull request may close these issues.

5 participants