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

Fixed joystick doc + properly deprecate pygame.Joystick.init/get_id #2363

Merged
merged 15 commits into from
Aug 21, 2023
Merged

Conversation

bilhox
Copy link
Contributor

@bilhox bilhox commented Jul 30, 2023

Hello,

In this PR, I corrected some errors I made while editing the joystick docs last time like :

  • Missing * for titles
  • Wrong mappings for certain buttons

I as well removed pygame.Joystick.init and pygame.Joystick.get_id, because the doc says that they were supposed to be removed since pygame 2.1, which wasn't the case. (Since it is my first time editing the code directly, I don't know if I removed it correctly)

@bilhox bilhox requested a review from a team as a code owner July 30, 2023 11:51
@yunline yunline added Code quality/robustness Code quality and resilience to changes docs joystick pygame.joystick labels Jul 30, 2023
Copy link
Contributor

@dr0id dr0id left a comment

Choose a reason for hiding this comment

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

LGTM

@zoldalma999
Copy link
Member

I don't think we should remove these functions yet.

I know the docs say these should have been removed in 2.1, but that is because when they were deprecated, 2.1 was supposed to allow breaking changes. Since then, we moved away to a different versioning system (I don't even know what the system is anymore tbh), but we for sure don't plan on breaking compatibility with older pygame versions.

These functions were also not properly deprecated. A lot of people don't check the docs, so they might easily miss deprecations if we only mention it there. Instead, we switched to raising deprecation warnings (again, after this note was added in the docs) in the code itself. Those are a lot harder to miss, so we are more confident in removing things that are marked this way.

So I think this PR should add deprecation warnings to these functions instead of removing them.

@dr0id
Copy link
Contributor

dr0id commented Aug 1, 2023

I don't think we should remove these functions yet.

I know the docs say these should have been removed in 2.1, but that is because when they were deprecated, 2.1 was supposed to allow breaking changes. Since then, we moved away to a different versioning system (I don't even know what the system is anymore tbh), but we for sure don't plan on breaking compatibility with older pygame versions.

These functions were also not properly deprecated. A lot of people don't check the docs, so they might easily miss deprecations if we only mention it there. Instead, we switched to raising deprecation warnings (again, after this note was added in the docs) in the code itself. Those are a lot harder to miss, so we are more confident in removing things that are marked this way.

So I think this PR should add deprecation warnings to these functions instead of removing them.

I see, from the user perspective I fully agree with the deprecation. So maybe raise the deprecation warning now and add a todo note from which version on one can remove the methods.

Copy link
Contributor

@dr0id dr0id left a comment

Choose a reason for hiding this comment

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

add deprecation warnings instead of removing those functions

@bilhox
Copy link
Contributor Author

bilhox commented Aug 1, 2023

Okay, I deprecated the methods in the code, shall I change the deprecation starting version from 2.0 to 2.3.1 ? Will this be commited to 2.3.1 branch ?

@bilhox bilhox changed the title Fixed joystick doc + removed pygame.Joystick.init/get_id Fixed joystick doc + properly deprecate pygame.Joystick.init/get_id Aug 1, 2023
Copy link
Contributor

@dr0id dr0id left a comment

Choose a reason for hiding this comment

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

there is a formatting issue in the c file

@bilhox
Copy link
Contributor Author

bilhox commented Aug 1, 2023

Okay, I fixed the problems (I hope).

@bilhox
Copy link
Contributor Author

bilhox commented Aug 3, 2023

Changed deprecation version from 2.3.1 to 2.4.0 and added a missing 'f' to a f-string in the joystick example code.
Ready to be merged.

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this PR ⭐

Overall nice PR, but left a few minor reviews

docs/reST/ref/joystick.rst Outdated Show resolved Hide resolved
docs/reST/ref/joystick.rst Outdated Show resolved Hide resolved
docs/reST/ref/joystick.rst Outdated Show resolved Hide resolved
examples/joystick.py Outdated Show resolved Hide resolved
examples/joystick.py Outdated Show resolved Hide resolved
@bilhox
Copy link
Contributor Author

bilhox commented Aug 20, 2023

So I fixed the minor problems, it should be fine.

@bilhox
Copy link
Contributor Author

bilhox commented Aug 20, 2023

Nevermind, a test failed but idk why

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

LGTM, but left another minor review, resolve at will

docs/reST/ref/joystick.rst Outdated Show resolved Hide resolved
@ankith26 ankith26 merged commit 1709782 into pygame-community:main Aug 21, 2023
26 of 27 checks passed
@ankith26 ankith26 added this to the 2.3.2 milestone Aug 21, 2023
ankith26 pushed a commit that referenced this pull request Sep 2, 2023
…`` (#2363)

* fixed docs + removed get_id / init methods

* removed my tests

* stubs update

* Properly deprecated methods

* fixed clang

* clang problems fixed

* doc problems fixed

* 2.3.1 to 2.4.0 deprecation version + fstring missing

* doc fix

* format fix

* doc fix

* format fix

* whitespace fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality/robustness Code quality and resilience to changes docs joystick pygame.joystick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants