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

Update annotation for pygame.Surface.blits() (and some other tidy ups) #2697

Merged
merged 9 commits into from Aug 31, 2021
Merged

Update annotation for pygame.Surface.blits() (and some other tidy ups) #2697

merged 9 commits into from Aug 31, 2021

Conversation

rethanon
Copy link
Contributor

Updated annotation for pygame.Surface.blits() to correct the keyword used and expected parameters.

Updated annotation for pygame.Surface.blits() to correct the keyword usage and expected parameters
Further update to annotation for pygame.Surface.blits() expected parameters
@rethanon
Copy link
Contributor Author

This is to fix issue #2693

@Starbuck5
Copy link
Contributor

Alright, this might not be as simple as I thought.

blit looks wrong as well. The area argument should take a _CanBeRect, and the destination argument looks like it should be a union of _Coordinate and _CanBeRect.

And those changes should be ported to blits as well, right, because blits takes a sequence of blit arguments.

Also, when I was checking the docs for this I noticed that the parentheses are unbalanced in the definitions!
Cmon blits:
Capture

If you want to fix that as well, it would be in docs/reST/ref/surface.rst. And you can regenerate the docs locally with setup.py docs to test your changes.

@rethanon
Copy link
Contributor Author

OK thanks, working on this, just having a problem with setup.py docs as it says I need Microsoft Visual C++ 14.2 or greater, so I'm having to install it.

@rethanon
Copy link
Contributor Author

rethanon commented Aug 27, 2021

I've made the updates but I can't seem to regenerate the docs, I get errors on both my Windows machine and my Mac when I run setup.py docs. In any case I was only removing a couple of ) nothing major.

@Starbuck5
Copy link
Contributor

Uh, that's weird.

setup.py docs should just build the docs, not pygame.

I think the types look good now.

For the docs I was thinking you would have to add an ( in the left to represent the sequence that the blit arguments are in. Would that be right?

@rethanon
Copy link
Contributor Author

rethanon commented Aug 27, 2021

I think the types look good now.

For the docs I was thinking you would have to add an ( in the left to represent the sequence that the blit arguments are in. Would that be right?

Oh man, yeah you're right, sorry. I only recently started using Black to format my code correctly and I misread the number of ( I had in the code I was testing, sorry amended now.

The issues with running setup.py docs are different on Mac vs my Windows PC so I'll look into them in greater detail later (although I the issue on Mac mentions an SDL2 error so I don't want to look at fixing that until I've finished helping with #2671)

@rethanon
Copy link
Contributor Author

rethanon commented Aug 27, 2021

Uh, that's weird.

setup.py docs should just build the docs, not pygame.

I've taken a closer look at the error I'm getting on my Windows machine and the first line starts with
WARNING, No "Setup" File Exists, Running "buildconfig/config.py"

I assume it's then trying to build pygame rather than just the docs.

@rethanon rethanon changed the title Update annotation for pygame.Surface.blits() Update annotation for pygame.Surface.blits() (and some other tidy ups) Aug 28, 2021
@rethanon
Copy link
Contributor Author

Added a few minor updates to annotations in mixer.pyi, also corrected a couple of minor typos in the mixer docs.

@@ -134,8 +134,8 @@

| :sl:`draw many images onto another`
| :sg:`blits(blit_sequence=(source, dest), ...), doreturn=1) -> [Rect, ...] or None`
Copy link
Contributor

Choose a reason for hiding this comment

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

This one still needs another parenthesis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow I didn't even notice that one! Thanks.

@@ -19,7 +19,7 @@ def init(
buffer: int = 512,
devicename: Optional[str] = None,
allowedchanges: int = 5,
) -> None: ...
) -> True: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably returns whether init was successful, not just True.

Also, if you're going to fix that you've got to fix the docs too, because they say it returns None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My fault, getting mixed up with which changes I'd made as I was looking at my fork on 2 different machines, think I committed the wrong one as the other one I'd changed to bool, not True. Also updated docs, apologies. I feel like a noob!

Minor updates to docs for mixer and surface.
@Starbuck5
Copy link
Contributor

Alright, I opened a broader issue about the mixer.init() thing, #2702, so I think it would be best to take the mixer.init() docs and types changes out of this PR.

Thanks for pointing that out though. I think it should be fixed across several modules, by changing the code to match the docs, rather than the other way around.

And the parentheses should be after blit_sequence=, since the sequence is the value of the keyword argument.

Also I guess I need to fix setup.py docs so that people who don't have the full setup to build pygame can build the docs.

@rethanon
Copy link
Contributor Author

Reverted changes for mixer docs and types and fixed parentheses in surface.blits docs. Thanks.

Copy link
Contributor

@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.

Looks good to go! Thanks for the PR! 🎈

Copy link
Contributor

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

This looks good to me as well. 👍

Thanks for contributing!

@Starbuck5 Starbuck5 added this to the 2.0.2 milestone Aug 31, 2021
@Starbuck5 Starbuck5 added docs types Type hint checking related tasks labels Aug 31, 2021
@Starbuck5 Starbuck5 merged commit 07d92e7 into pygame:main Aug 31, 2021
@Starbuck5 Starbuck5 added the Surface pygame.Surface label Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Surface pygame.Surface types Type hint checking related tasks
Projects
None yet
3 participants