8000 Added kwarg handling to rect.c by jonotassia · Pull Request #3872 · pygame/pygame · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Added kwarg handling to rect.c #3872

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and 8000 privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jun 3, 2023
Merged

Added kwarg handling to rect.c #3872

merged 8 commits into from
Jun 3, 2023

Conversation

jonotassia
Copy link
Contributor

Implemented the below changes to display.c functions (per #3852) to add kwarg support:

  • Kwarg support in display.c
  • Doc updates in display.rst
  • Test cases updated and passed

Functions covered:

  • pg_rect_clipline
  • pg_rect_scale_by
  • pg_rect_scale_by_ip
  • pg_rect_unionall
  • pg_rect_unionall_ip
  • pg_rect_collidelist
  • pg_rect_collidelistall
  • pg_rect_collidedict
  • pg_rect_collidedictall

@jonotassia
Copy link
Contributor Author

I added and ran tests for all of 8000 these, and they are working across the board, but I'm not sure how best to handle keyword arguments when the function is overloaded in the functions below:

  • pg_rect_clipline
  • pg_rect_scale_by
  • pg_rect_scale_by_ip

In these cases, arguments could be passed in a number of ways:

clipline

    @overload
    def clipline(
        self, x1: float, x2: float, x3: float, x4: float
    ) -> Union[Tuple[Tuple[int, int], Tuple[int, int]], Tuple[()]]: ...
    @overload
    def clipline(
        self, first_coordinate: Coordinate, second_coordinate: Coordinate
    ) -> Union[Tuple[Tuple[int, int], Tuple[int, int]], Tuple[()]]: ...
    @overload
    def clipline(
        self, rect_arg: RectValue
    ) -> Union[Tuple[Tuple[int, int], Tuple[int, int]], Tuple[()]]: ...

scale_by and scale_by_ip

    @overload
    def scale_by(self, x: float, y: float) -> Rect: ...
    @overload
    def scale_by(self, scale_by: Coordinate) -> Rect: ...
    @overload
    def scale_by_ip(self, x: float, y: float) -> None: ...
    @overload
    def scale_by_ip(self, scale_by: Coordinate) -> None: ...

I'd imagine most users would expect to be able to pass the arguments in the overloaded functions as well, ie scale_by(scale_by=(x, y)) as well as scale_by(x=x, y=y), but I'm not sure how best to handle the different ways to interface with the function without adding in a number of other variables and explicitly handling them, which seems a little messy.

Do you have any recommendations on how to handle kwargs with overloaded functions?

@jonotassia jonotassia mentioned this pull request May 22, 2023
Closed
18 tasks
illume
illume reviewed May 24, 2023
View reviewed changes
illume
illume reviewed May 24, 2023
View reviewed changes
illume
illume reviewed May 24, 2023
View reviewed changes
@illume
Copy link
Member
illume commented May 24, 2023

Very good. I left a few notes for your consideration.

As to your question…

I'd imagine most users would expect to be able to pass the arguments in the overloaded functions as well, ie scale_by(scale_by=(x, y)) as well as scale_by(x=x, y=y), but I'm not sure how best to handle the different ways to interface with the function without adding in a number of other variables and explicitly handling them, which seems a little messy.

Do you have any recommendations on how to handle kwargs with overloaded functions?

Yeah, people will probably want to call them like that. Good point.

You add all of the keyword arguments. Then if they are not used they are set to NULL if not used. So you can check the user has done something sensible. Raise a ValueError if an unsupported combination is used. Like x and scale_by but not y.

@jonotassia
Copy link
Contributor Author

Thanks for the suggestion. I've gone ahead and made changes based on your suggestion for scale_by and scale_by_ip, and added some additional test cases to make sure it doesn't accept combinations it shouldn't.

I'll go ahead and update the clipline kwarg handling tomorrow.

@jonotassia
Copy link
Contributor Author

Alright, this should all be in working order now (though I'll keep an eye on the automated checks in case there are any compilation errors).

Updates:

  1. I've now added handling for all the different overloaded versions of clipline, scale_by, and scale_by_ip. It added more lines of code than I initially expected to make sure the keywords were being handled before hitting PyArg_ParseTupleAndKeywords, so open to any suggestions if you can see a better way to account for the different keyword options.

  2. I've also added test cases to ensure exceptions are appropriately raised if a user passes incorrect inputs to the functions via kwargs.

  3. Lastly, I've updated the stub files per your previous suggestions for collidelist and unionlistall so that the keywords for both of those are simply rects rather than rect_list and rect_sequence.

@jonotassia
Copy link
Contributor Author

Looks like Python versions over 3.7 are failing the below test cases across the board. I'll take a look next week and try to get that fixed.

  ======================================================================
  ERROR: test_scale_by_ip__kwargs (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 1072, in test_scale_by_ip__kwargs
      r2.scale_by_ip(x=2, y=4)
  SystemError: Function returned a NULL result without setting an exception
  
  ======================================================================
  ERROR: test_scale_by_ip__larger (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 1036, in test_scale_by_ip__larger
      r2.scale_by_ip(2)
  SystemError: Function returned a NULL result without setting an exception
  
  ======================================================================
  ERROR: test_scale_by_ip__smaller (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 1050, in test_scale_by_ip__smaller
      r2.scale_by_ip(0.5)
  SystemError: Function returned a NULL result without setting an exception
  
  ======================================================================
  ERROR: test_scale_by_ip__subzero (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 1063, in test_scale_by_ip__subzero
      r.scale_by_ip(0)
  SystemError: Function returned a NULL result without setting an exception
  
  ======================================================================
  FAIL: test_scale_by__kwarg_exceptions (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle using
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 867, in test_scale_by__kwarg_exceptions
      r.scale_by(scale_by=(1, 2), y=1)
  AssertionError: SystemError not raised
  
  ======================================================================
  FAIL: test_scale_by__larger (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 890, in test_scale_by__larger
      self.assertEqual(r.left - 3, r2.left)
  AssertionError: -1 != 2
  
  ======================================================================
  FAIL: test_scale_by__larger_kwargs (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 926, in test_scale_by__larger_kwargs
      self.assertEqual(r.left - 3, r2.left)
  AssertionError: -1 != 2
  
  ======================================================================
  FAIL: test_scale_by__larger_kwargs_scale_by (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 908, in test_scale_by__larger_kwargs_scale_by
      self.assertEqual(r.left - 3, r2.left)
  AssertionError: -1 != 2
  
  ======================================================================
  FAIL: test_scale_by__larger_single_argument (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 837, in test_scale_by__larger_single_argument
      self.assertEqual(r.left - 3, r2.left)
  AssertionError: -1 != 2
  
  ======================================================================
  FAIL: test_scale_by__larger_single_argument_kwarg (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle using
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 851, in test_scale_by__larger_single_argument_kwarg
      self.assertEqual(r.left - 3, r2.left)
  AssertionError: -1 != 2
  
  ======================================================================
  FAIL: test_scale_by__smaller (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 941, in test_scale_by__smaller
      self.assertEqual(r.left + 2, r2.left)
  AssertionError: 4 != 2
  
  ======================================================================
  FAIL: test_scale_by__smaller_single_argument (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 875, in test_scale_by__smaller_single_argument
      self.assertEqual(r.left + 2, r2.left)
  AssertionError: 4 != 2
  
  ======================================================================
  FAIL: test_scale_by__subzero (pygame.tests.rect_test.RectTypeTest)
  The scale method scales around the center of the rectangle
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/tmp/tmp.bjwhdYnITL/venv/site-packages/pygame/tests/rect_test.py", line 957, in test_scale_by__subzero
      self.assertEqual(r.centerx - r.w * 10 / 2, rx1.x)
  AssertionError: -25.0 != 2

@illume
Copy link
Member
illume commented May 31, 2023

What do you think about removing the scaleby changes, and then the other stuff can be merged in?
I guess it's only the scaleby one that is having problems.

@jonotassia
Copy link
Contributor Author
jonotassia commented May 31, 2023 •
edited
Loading

I found the issue this morning: I accidentally initialised the temporary values as ints rather than floats in scale_by_ip , so that would have lead to PyArg_ParseTupleAndKeywords to fail and return NULL, and would probably also explain the large difference in return values in some of the test cases.

I'm not sure why the handling was different Python 3.8 and higher before, but it looks like everything is passing now in the automated checks except for Ubuntu 3.12dev, but it seems like that is an import error that seems unrelated to these changes?

ERROR: Exception:
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/cli/base_command.py", line 169, in exc_logging_wrapper
    status = run_func(*args)
             ^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/cli/req_command.py", line 248, in wrapper
    return func(self, options, args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/commands/install.py", line 377, in run
    requirement_set = resolver.resolve(
                      ^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/resolution/resolvelib/resolver.py", line 92, in resolve
    result = self._result = resolver.resolve(
                            ^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_vendor/resolvelib/resolvers.py", line 546, in resolve
    state = resolution.resolve(requirements, max_rounds=max_rounds)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_vendor/resolvelib/resolvers.py", line 397, in resolve
    self._add_to_criteria(self.state.criteria, r, parent=None)
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_vendor/resolvelib/resolvers.py", line 173, in _add_to_criteria
    if not criterion.candidates:
           ^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_vendor/resolvelib/structs.py", line 156, in __bool__
    return bool(self._sequence)
           ^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/resolution/resolvelib/found_candidates.py", line 155, in __bool__
    return any(self)
           ^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/resolution/resolvelib/found_candidates.py", line 143, in <genexpr>
    return (c for c in iterator if id(c) not in self._incompatible_ids)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/resolution/resolvelib/found_candidates.py", line 47, in _iter_built
    candidate = func()
                ^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/resolution/resolvelib/factory.py", line 206, in _make_candidate_from_link
    self._link_candidate_cache[link] = LinkCandidate(
                                       ^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/resolution/resolvelib/candidates.py", line 293, in __init__
    super().__init__(
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/resolution/resolvelib/candidates.py", line 156, in __init__
    self.dist = self._prepare()
                ^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/resolution/resolvelib/candidates.py", line 225, in _prepare
    dist = self._prepare_distribution()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/resolution/resolvelib/candidates.py", line 304, in _prepare_distribution
    return preparer.prepare_linked_requirement(self._ireq, parallel_builds=True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/operations/prepare.py", line 516, in prepare_linked_requirement
    return self._prepare_linked_requirement(req, parallel_builds)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/operations/prepare.py", line 631, in _prepare_linked_requirement
    dist = _get_prepared_distribution(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/operations/prepare.py", line 69, in _get_prepared_distribution
    abstract_dist.prepare_distribution_metadata(
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/distributions/sdist.py", line 48, in prepare_distribution_metadata
    self._install_build_reqs(finder)
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/distributions/sdist.py", line 118, in _install_build_reqs
    build_reqs = self._get_build_requires_wheel()
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/distributions/sdist.py", line 95, in _get_build_requires_wheel
    return backend.get_requires_for_build_wheel()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_internal/utils/misc.py", line 692, in get_requires_for_build_wheel
    return super().get_requires_for_build_wheel(config_settings=cs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_impl.py", line 166, in get_requires_for_build_wheel
    return self._call_hook('get_requires_for_build_wheel', {
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_impl.py", line 321, in _call_hook
    raise BackendUnavailable(data.get('traceback', ''))
pip._vendor.pyproject_hooks._impl.BackendUnavailable: Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/site-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 77, in _build_backend
    obj = import_module(mod_path)
          ^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.0-beta.1/x64/lib/python3.12/importlib/__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1293, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1266, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1216, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 400, in _call_with_frames_removed
  File "<frozen importlib._bootstrap>", line 1293, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1266, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1237, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 841, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 994, in exec_module
  File "<frozen importlib._bootstrap>", line 400, in _call_with_frames_removed
  File "/tmp/pip-build-env-i_plq3gb/overlay/lib/python3.12/site-packages/setuptools/__init__.py", line 10, in <module>
    import distutils.core
ModuleNotFoundError: No module named 'distutils'

illume
illume approved these changes Jun 3, 2023
View reviewed changes
Copy link
Member
@illume illume left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks. 🎉🎈

@illume illume merged commit d6718b8 into pygame:main Jun 3, 2023
@jonotassia jonotassia deleted the add-kwarg-rect branch June 5, 2023 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@illume illume illume approved these changes

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
@jonotassia @illume
0