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

fixing Set and CGI signatures #761

Merged
merged 6 commits into from
Feb 22, 2022
Merged

fixing Set and CGI signatures #761

merged 6 commits into from
Feb 22, 2022

Conversation

HoneyryderChuck
Copy link
Contributor

@HoneyryderChuck HoneyryderChuck commented Aug 23, 2021

  • Set: set the same type of generic element as for arrays;
  • CGI: added missing modules (Cookie, Util, Escape...)

@HoneyryderChuck HoneyryderChuck changed the title fixing Set signatures fixing Set and CGI signatures Aug 23, 2021
@HoneyryderChuck
Copy link
Contributor Author

@soutaro I see there's already an open PR for Tempfile. Is there a solution thought out for delegate classes? Both these and Struct seem to be unaddressed when it comes to dynamic attributes.

@soutaro
Copy link
Member

soutaro commented Aug 26, 2021

We don't have support for delegate classes. I'm not sure what is the best way...

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

@HoneyryderChuck Could you update the type of json_creates?

And we are waiting for #767 get merged, right?

@HoneyryderChuck
Copy link
Contributor Author

And we are waiting for #767 get merged, right?

I'd say #767 is also dependent on #765 . Nevertheless, I'd be ok with merging it, as it's an acceptable workaround (and I just checked that sorbet did the same).

However, it's a hack, as File is not the superclass of Tempfile (Delegator is), and this will include File class methods in Tempfile, which is wrong.

This type of more "dynamic-ish" typing will become more important to figure out as the stdlib signature set completes, as I think it's relevant for a few of them such as delegate, struct and ostruct (and method_missing delegations, perhaps). I've done a "draft" in #765 on how it could look like internally, but I think that the top-level API should look different and have specific RBS grammar.

@HoneyryderChuck
Copy link
Contributor Author

@soutaro I think this is ready to be merged.

On the proposal above ☝️ , do you have some thoughts?

@HoneyryderChuck
Copy link
Contributor Author

@soutaro ping ☝️

@HoneyryderChuck
Copy link
Contributor Author

@soutaro just rebased the latest changes, can you give it another try?

@soutaro soutaro merged commit 0bb2df0 into ruby:master Feb 22, 2022
@soutaro
Copy link
Member

soutaro commented Feb 22, 2022

@HoneyryderChuck Done! Fixed some issues myself and pushed to your repo. Thanks! 🙏

@HoneyryderChuck HoneyryderChuck deleted the fix-set branch February 22, 2022 12:55
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.

2 participants