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

Fix n_args_min on many MP_DEFINE_CONST_FUN_OBJ_KW() #5439

Merged
merged 3 commits into from
Oct 8, 2021

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented Oct 7, 2021

#5407 and #5405 discovered that some routines and methods were requiring positional-only args for some required arguments. I think this was mainly a misunderstanding of what n_args_min in MP_DEFINE_CONST_FUN_OBJ_KW(obj_name, n_args_min, fun_name) meant. Thanks to @jepler for pointing this out. n_args_min here means the minimum of positional-only arguments required. It does not need to be used to specify the minimum number of arguments. MP_ARG_REQUIRED takes care of requiring any particular argument, and gives a better error if the argument missing (e.g., instead of "function missing 1 required positional arguments", we get an error like "'buffer' argument required").

In general, n_args_min can always be 0, when the function is a plain function, or 1, when it's a method. The one argument is self in the latter case.

I did a little other cleanup, fixing some incorrect Optional[int] args and using buffer consistently instead of buf in busio and bitbangio,and regularzing some arg parsing in socketpool and storage.

I tested a number of these changes in the REPL to make sure they were still requiring the right args, and could be used with and without keywords, including the trickier changes in socketpool and storage.

@jepler
Copy link
Member

jepler commented Oct 7, 2021

Thanks! This is extensive, I don't know that I can fairly review it but I'll try.

jepler
jepler previously approved these changes Oct 7, 2021
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

that actually went faster than I thought. No testing performed, one potential nit raised.

shared-bindings/busio/SPI.c Outdated Show resolved Hide resolved
@dhalbert
Copy link
Collaborator Author

dhalbert commented Oct 8, 2021

@tannewt Correcting the doc revealed that I2C was using in_buffer and out_buffer for the write_readinto(), and SPI was using buffer_in and buffer_out. However, because of the original bugs this PR fixes, the I2C keywords were not usable at all, and for SPI, only the second buffer arg could be specified with a keyword.

I found no uses of any of these keyword args in the bundle. @jepler and I discussed it briefly, and I made the SPI ones match the I2C ones: in_buffer and out_buffer. Technically this is a breaking change, but only on the second SPI arg keyword, and I think no one ever used it. I could do it the other way round, and it would not be a breaking change at all, because of the original bug. But I like in and out as prefixes rather than suffixes. Your comments welcome.

rp2io does use suffixes rather than prefixes.

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks, I didn't spot any problems. Hope this is the last round of doc fixes, thanks so much for taking the time to get it right.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

@dhalbert Thanks for fixing the inconsistency. I'm ok with the minor breakage.

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.

Positional args with keywords not working in native methods
3 participants