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

Misc improvements #162

Merged
merged 8 commits into from
Aug 29, 2023
Merged

Misc improvements #162

merged 8 commits into from
Aug 29, 2023

Conversation

mullermp
Copy link
Contributor

RBS struct members
Refactor interceptor apply out of interceptor list
Rename yardType to docType
Extra RBS in hearth + some minor tweaks

other minor stuff.

Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

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

LGTM - just minor comments

String rbsType = "Array[" + member.getProperty("rbsType").get() + "]";
String yardType = "Array<" + member.getProperty("yardType").get() + ">";
return createSymbolBuilder(shape, getDefaultShapeName(shape, "List__"), rbsType, yardType, moduleName)
String rbsType = "::Array[" + member.getProperty("rbsType").get() + "]";
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope of this PR - but I think related to my comment from previous PR - I think maybe we shoudl define a RubyType class and create instances for things like Array. that will know how to do the doc types, rbs, contarints, ect.... I think this applies here and in the middleware and possibly in other places as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what that would look like... But yeah we can potentially do that outside this change.

hearth/lib/hearth/middleware/build.rb Outdated Show resolved Hide resolved
@mullermp mullermp merged commit 887c288 into main Aug 29, 2023
20 checks passed
@mullermp mullermp deleted the misc branch August 29, 2023 23:47
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