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

Suppress very common configure() type checker error #1177

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

pantheraleo-7
Copy link
Contributor

@pantheraleo-7 pantheraleo-7 commented Dec 12, 2024

Since the default value was a str, passing a dict gave type errors. Since the codebase has very minimal static typing [of only return types], this approach is a clean and pythonic way of suppressing the [false-positive] errors without introducing strict static typing of the parameters.

@pantheraleo-7 pantheraleo-7 force-pushed the suppress-type-errors branch 3 times, most recently from 315954e to 1fde8b6 Compare December 12, 2024 04:45
@davidplowman
Copy link
Collaborator

Hi, thanks for the PR. I don't know why there was a test failure when running the change, but I don't think it's related. I may just need to increase a time out or something. But could I ask you to make a couple of easy changes and I think we'll be good to merge this?

  1. Could you change the destination branch to raspberrypi/next? We always push new stuff onto the next branch first.
  2. Could you squash to two commits together into one? The changes are sufficiently small that I think it will be easier for folks reviewing the history if there's just one. Please could you sign your commit as well (check here).

Thanks very much!

@pantheraleo-7 pantheraleo-7 changed the base branch from main to next December 16, 2024 18:49
@pantheraleo-7 pantheraleo-7 force-pushed the suppress-type-errors branch 2 times, most recently from 2ee0202 to f1f9dd2 Compare December 16, 2024 19:07
@pantheraleo-7
Copy link
Contributor Author

I am sorry that I didn't read the contributing guide.

@davidplowman
Copy link
Collaborator

I am sorry that I didn't read the contributing guide.

No need to apologise, always happy to receive contributions! 😃

Tests are all good, so just one thing to check. If wonder if we should add in

        if camera_config is None:
            camera_config = "preview"

just to 100% guarantee the same behaviour if someone passes in no config at all. Other than that, I'm happy to merge this. Thanks!

@pantheraleo-7
Copy link
Contributor Author

This is what I initially did but if you see L1047-L1048, there's already a check for camera_config=None which then get set to the preview configuration. So doing this is actually redundant.

@davidplowman
Copy link
Collaborator

Yes, thanks, I actually noticed that too, but a (very subtle) different thing will happen. In one case you'll get this, and in the other, the line you pointed out.

To be honest, the whole configuration thing is a bit too twisty these days, but OTOH I don't particularly have the enthusiasm to change stuff, so it is what it is. If we add that line, then it really is for sure 100% equivalent to what was there before!!

Signed-off-by: Asadullah Shaikh <[email protected]>
@pantheraleo-7
Copy link
Contributor Author

Is this style okay or do you want a separate if clause?

@pantheraleo-7
Copy link
Contributor Author

pantheraleo-7 commented Dec 17, 2024

Also what's the difference between these two? self.preview_configuration is same as self.create_preview_configuration() as per L273.

Correct me if I'm wrong but isn't L274 an [AttributeError] error, as dicts don't have enable_raw method on them, which is getting caught by the generic except clause on L279?
Edit: Actually it's a CameraConfiguration object, so no errors. The property setter is getting called with the dict as the argument. So it's a false positive from pyright. TBF calling the setter in the __init__ doesn't seem like good practice, especially when this is a good to place to instead initialise self.*_configuration_ s.

@davidplowman
Copy link
Collaborator

I think the difference is that self.preview_configuration can be updated by a user, and then the string "preview" refers to this. create_preview_configuration() always gives you a vanilla default one. There are, slightly unhappily, two different way of configuring everything, one using dicts and the other these configuration objects, so it is all a bit confusing. (Mostly I just use the dict method because I don't really understand the other one!)

I don't think we get an error there because those configuration objects are slightly weird magic things. But like I said, I don't particularly understand...

@pantheraleo-7
Copy link
Contributor Author

pantheraleo-7 commented Dec 17, 2024

Yeah I got mixed up in the self.*_configurations of the __init__ and the @property ones.

Here's the summary:

  • "preview" will get you a CameraConfiguration object
  • None will get you a dict object

@davidplowman davidplowman merged commit 081692e into raspberrypi:next Dec 17, 2024
4 checks passed
@pantheraleo-7 pantheraleo-7 deleted the suppress-type-errors branch December 17, 2024 18:32
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.

2 participants