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

use optional record keys for several API options #1900

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HoneyryderChuck
Copy link
Contributor

No description provided.

@HoneyryderChuck HoneyryderChuck force-pushed the optional-keys-improv branch 2 times, most recently from eaf2497 to 7d4f483 Compare June 21, 2024 10:39
@HoneyryderChuck HoneyryderChuck changed the title DRAFT: use optional record keys for several API options use optional record keys for several API options Jun 21, 2024
@HoneyryderChuck HoneyryderChuck force-pushed the optional-keys-improv branch 12 times, most recently from bd4c4af to ca9e03a Compare June 26, 2024 10:28
@HoneyryderChuck
Copy link
Contributor Author

@soutaro it's ready for review 🙏

@soutaro
Copy link
Member

soutaro commented Jun 27, 2024

Thank you for your work! 🎉 I'm totally good for using optional records for return values.

However, using the records instead of Hash for options may result in reporting more type errors.

opts = {}
# Set up opts
CGI.new(opts)        # Type error because it's a Hash, not a record

What do you think about this?
I'm thinking of if having additional rules for Hash -> record conversion in Steep would help.

@HoneyryderChuck
Copy link
Contributor Author

I'll run some tests locally to see if this impacts rbs runtime type check significantly. Nevertheless, I think there's great value in being precise on the options you accept in a hash as a function argument, particularly in examples such as ssl context, or URI build functions. That's definitely something steep could be a bit smarter about 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants